diff options
-rw-r--r-- | crates/hir/src/diagnostics.rs | 11 | ||||
-rw-r--r-- | crates/hir_ty/src/diagnostics.rs | 48 | ||||
-rw-r--r-- | crates/hir_ty/src/diagnostics/expr.rs | 70 | ||||
-rw-r--r-- | crates/ide/src/diagnostics.rs | 3 | ||||
-rw-r--r-- | crates/ide/src/diagnostics/fixes.rs | 33 |
5 files changed, 150 insertions, 15 deletions
diff --git a/crates/hir/src/diagnostics.rs b/crates/hir/src/diagnostics.rs index 447faa04f..f7fd3e237 100644 --- a/crates/hir/src/diagnostics.rs +++ b/crates/hir/src/diagnostics.rs | |||
@@ -5,5 +5,14 @@ pub use hir_expand::diagnostics::{ | |||
5 | }; | 5 | }; |
6 | pub use hir_ty::diagnostics::{ | 6 | pub use hir_ty::diagnostics::{ |
7 | IncorrectCase, MismatchedArgCount, MissingFields, MissingMatchArms, MissingOkOrSomeInTailExpr, | 7 | IncorrectCase, MismatchedArgCount, MissingFields, MissingMatchArms, MissingOkOrSomeInTailExpr, |
8 | NoSuchField, RemoveThisSemicolon, | 8 | NoSuchField, RemoveThisSemicolon, ReplaceFilterMapNextWithFindMap, |
9 | }; | 9 | }; |
10 | |||
11 | // PHIL: | ||
12 | // hir/src/diagnostics.rs - just pub uses the type from hir_ty::diagnostics (DONE) | ||
13 | // hir_ty/src/diagnostics.rs - defines the type (DONE) | ||
14 | // hir_ty/src/diagnostics.rs - plus a test (DONE) <--- one example found, need to copy the not-applicable tests from the assist version | ||
15 | // ide/src/diagnostics.rs - define handler for when this diagnostic is raised (DONE) | ||
16 | |||
17 | // ide/src/diagnostics/fixes.rs - pulls in type from hir, and impls DiagnosticWithFix (TODO) | ||
18 | // hir_ty/src/diagnostics/expr.rs - do the real work (TODO) | ||
diff --git a/crates/hir_ty/src/diagnostics.rs b/crates/hir_ty/src/diagnostics.rs index 247da43f2..b4cf81c10 100644 --- a/crates/hir_ty/src/diagnostics.rs +++ b/crates/hir_ty/src/diagnostics.rs | |||
@@ -247,7 +247,7 @@ impl Diagnostic for RemoveThisSemicolon { | |||
247 | 247 | ||
248 | // Diagnostic: break-outside-of-loop | 248 | // Diagnostic: break-outside-of-loop |
249 | // | 249 | // |
250 | // This diagnostic is triggered if `break` keyword is used outside of a loop. | 250 | // This diagnostic is triggered if the `break` keyword is used outside of a loop. |
251 | #[derive(Debug)] | 251 | #[derive(Debug)] |
252 | pub struct BreakOutsideOfLoop { | 252 | pub struct BreakOutsideOfLoop { |
253 | pub file: HirFileId, | 253 | pub file: HirFileId, |
@@ -271,7 +271,7 @@ impl Diagnostic for BreakOutsideOfLoop { | |||
271 | 271 | ||
272 | // Diagnostic: missing-unsafe | 272 | // Diagnostic: missing-unsafe |
273 | // | 273 | // |
274 | // This diagnostic is triggered if operation marked as `unsafe` is used outside of `unsafe` function or block. | 274 | // This diagnostic is triggered if an operation marked as `unsafe` is used outside of an `unsafe` function or block. |
275 | #[derive(Debug)] | 275 | #[derive(Debug)] |
276 | pub struct MissingUnsafe { | 276 | pub struct MissingUnsafe { |
277 | pub file: HirFileId, | 277 | pub file: HirFileId, |
@@ -295,7 +295,7 @@ impl Diagnostic for MissingUnsafe { | |||
295 | 295 | ||
296 | // Diagnostic: mismatched-arg-count | 296 | // Diagnostic: mismatched-arg-count |
297 | // | 297 | // |
298 | // This diagnostic is triggered if function is invoked with an incorrect amount of arguments. | 298 | // This diagnostic is triggered if a function is invoked with an incorrect amount of arguments. |
299 | #[derive(Debug)] | 299 | #[derive(Debug)] |
300 | pub struct MismatchedArgCount { | 300 | pub struct MismatchedArgCount { |
301 | pub file: HirFileId, | 301 | pub file: HirFileId, |
@@ -347,7 +347,7 @@ impl fmt::Display for CaseType { | |||
347 | 347 | ||
348 | // Diagnostic: incorrect-ident-case | 348 | // Diagnostic: incorrect-ident-case |
349 | // | 349 | // |
350 | // This diagnostic is triggered if item name doesn't follow https://doc.rust-lang.org/1.0.0/style/style/naming/README.html[Rust naming convention]. | 350 | // This diagnostic is triggered if an item name doesn't follow https://doc.rust-lang.org/1.0.0/style/style/naming/README.html[Rust naming convention]. |
351 | #[derive(Debug)] | 351 | #[derive(Debug)] |
352 | pub struct IncorrectCase { | 352 | pub struct IncorrectCase { |
353 | pub file: HirFileId, | 353 | pub file: HirFileId, |
@@ -386,6 +386,31 @@ impl Diagnostic for IncorrectCase { | |||
386 | } | 386 | } |
387 | } | 387 | } |
388 | 388 | ||
389 | // Diagnostic: replace-filter-map-next-with-find-map | ||
390 | // | ||
391 | // This diagnostic is triggered when `.filter_map(..).next()` is used, rather than the more concise `.find_map(..)`. | ||
392 | #[derive(Debug)] | ||
393 | pub struct ReplaceFilterMapNextWithFindMap { | ||
394 | pub file: HirFileId, | ||
395 | pub filter_map_expr: AstPtr<ast::Expr>, | ||
396 | pub next_expr: AstPtr<ast::Expr>, | ||
397 | } | ||
398 | |||
399 | impl Diagnostic for ReplaceFilterMapNextWithFindMap { | ||
400 | fn code(&self) -> DiagnosticCode { | ||
401 | DiagnosticCode("replace-filter-map-next-with-find-map") | ||
402 | } | ||
403 | fn message(&self) -> String { | ||
404 | "replace filter_map(..).next() with find_map(..)".to_string() | ||
405 | } | ||
406 | fn display_source(&self) -> InFile<SyntaxNodePtr> { | ||
407 | InFile { file_id: self.file, value: self.filter_map_expr.clone().into() } | ||
408 | } | ||
409 | fn as_any(&self) -> &(dyn Any + Send + 'static) { | ||
410 | self | ||
411 | } | ||
412 | } | ||
413 | |||
389 | #[cfg(test)] | 414 | #[cfg(test)] |
390 | mod tests { | 415 | mod tests { |
391 | use base_db::{fixture::WithFixture, FileId, SourceDatabase, SourceDatabaseExt}; | 416 | use base_db::{fixture::WithFixture, FileId, SourceDatabase, SourceDatabaseExt}; |
@@ -644,4 +669,19 @@ fn foo() { break; } | |||
644 | "#, | 669 | "#, |
645 | ); | 670 | ); |
646 | } | 671 | } |
672 | |||
673 | #[test] | ||
674 | fn replace_missing_filter_next_with_find_map() { | ||
675 | check_diagnostics( | ||
676 | r#" | ||
677 | fn foo() { | ||
678 | let m = [1, 2, 3] | ||
679 | .iter() | ||
680 | .filter_map(|x| if *x == 2 { Some (4) } else { None }) | ||
681 | .next(); | ||
682 | //^^^ Replace .filter_map(..).next() with .find_map(..) | ||
683 | } | ||
684 | "#, | ||
685 | ); | ||
686 | } | ||
647 | } | 687 | } |
diff --git a/crates/hir_ty/src/diagnostics/expr.rs b/crates/hir_ty/src/diagnostics/expr.rs index 107417c27..170d23178 100644 --- a/crates/hir_ty/src/diagnostics/expr.rs +++ b/crates/hir_ty/src/diagnostics/expr.rs | |||
@@ -24,6 +24,8 @@ pub(crate) use hir_def::{ | |||
24 | LocalFieldId, VariantId, | 24 | LocalFieldId, VariantId, |
25 | }; | 25 | }; |
26 | 26 | ||
27 | use super::ReplaceFilterMapNextWithFindMap; | ||
28 | |||
27 | pub(super) struct ExprValidator<'a, 'b: 'a> { | 29 | pub(super) struct ExprValidator<'a, 'b: 'a> { |
28 | owner: DefWithBodyId, | 30 | owner: DefWithBodyId, |
29 | infer: Arc<InferenceResult>, | 31 | infer: Arc<InferenceResult>, |
@@ -39,7 +41,18 @@ impl<'a, 'b> ExprValidator<'a, 'b> { | |||
39 | ExprValidator { owner, infer, sink } | 41 | ExprValidator { owner, infer, sink } |
40 | } | 42 | } |
41 | 43 | ||
44 | fn bar() { | ||
45 | // LOOK FOR THIS | ||
46 | let m = [1, 2, 3] | ||
47 | .iter() | ||
48 | .filter_map(|x| if *x == 2 { Some(4) } else { None }) | ||
49 | .next(); | ||
50 | } | ||
51 | |||
42 | pub(super) fn validate_body(&mut self, db: &dyn HirDatabase) { | 52 | pub(super) fn validate_body(&mut self, db: &dyn HirDatabase) { |
53 | // DO NOT MERGE: just getting something working for now | ||
54 | self.check_for_filter_map_next(db); | ||
55 | |||
43 | let body = db.body(self.owner.into()); | 56 | let body = db.body(self.owner.into()); |
44 | 57 | ||
45 | for (id, expr) in body.exprs.iter() { | 58 | for (id, expr) in body.exprs.iter() { |
@@ -150,20 +163,58 @@ impl<'a, 'b> ExprValidator<'a, 'b> { | |||
150 | } | 163 | } |
151 | } | 164 | } |
152 | 165 | ||
153 | fn validate_call(&mut self, db: &dyn HirDatabase, call_id: ExprId, expr: &Expr) -> Option<()> { | 166 | fn check_for_filter_map_next(&mut self, db: &dyn HirDatabase) { |
167 | let body = db.body(self.owner.into()); | ||
168 | let mut prev = None; | ||
169 | |||
170 | for (id, expr) in body.exprs.iter() { | ||
171 | if let Expr::MethodCall { receiver, method_name, args, .. } = expr { | ||
172 | let method_name_hack_do_not_merge = format!("{}", method_name); | ||
173 | |||
174 | if method_name_hack_do_not_merge == "filter_map" && args.len() == 1 { | ||
175 | prev = Some((id, args[0])); | ||
176 | continue; | ||
177 | } | ||
178 | |||
179 | if method_name_hack_do_not_merge == "next" { | ||
180 | if let Some((filter_map_id, filter_map_args)) = prev { | ||
181 | if *receiver == filter_map_id { | ||
182 | let (_, source_map) = db.body_with_source_map(self.owner.into()); | ||
183 | if let (Ok(filter_map_source_ptr), Ok(next_source_ptr)) = ( | ||
184 | source_map.expr_syntax(filter_map_id), | ||
185 | source_map.expr_syntax(id), | ||
186 | ) { | ||
187 | self.sink.push(ReplaceFilterMapNextWithFindMap { | ||
188 | file: filter_map_source_ptr.file_id, | ||
189 | filter_map_expr: filter_map_source_ptr.value, | ||
190 | next_expr: next_source_ptr.value, | ||
191 | }); | ||
192 | } | ||
193 | } | ||
194 | } | ||
195 | } | ||
196 | } | ||
197 | prev = None; | ||
198 | } | ||
199 | } | ||
200 | |||
201 | fn validate_call(&mut self, db: &dyn HirDatabase, call_id: ExprId, expr: &Expr) { | ||
154 | // Check that the number of arguments matches the number of parameters. | 202 | // Check that the number of arguments matches the number of parameters. |
155 | 203 | ||
156 | // FIXME: Due to shortcomings in the current type system implementation, only emit this | 204 | // FIXME: Due to shortcomings in the current type system implementation, only emit this |
157 | // diagnostic if there are no type mismatches in the containing function. | 205 | // diagnostic if there are no type mismatches in the containing function. |
158 | if self.infer.type_mismatches.iter().next().is_some() { | 206 | if self.infer.type_mismatches.iter().next().is_some() { |
159 | return None; | 207 | return; |
160 | } | 208 | } |
161 | 209 | ||
162 | let is_method_call = matches!(expr, Expr::MethodCall { .. }); | 210 | let is_method_call = matches!(expr, Expr::MethodCall { .. }); |
163 | let (sig, args) = match expr { | 211 | let (sig, args) = match expr { |
164 | Expr::Call { callee, args } => { | 212 | Expr::Call { callee, args } => { |
165 | let callee = &self.infer.type_of_expr[*callee]; | 213 | let callee = &self.infer.type_of_expr[*callee]; |
166 | let sig = callee.callable_sig(db)?; | 214 | let sig = match callee.callable_sig(db) { |
215 | Some(sig) => sig, | ||
216 | None => return, | ||
217 | }; | ||
167 | (sig, args.clone()) | 218 | (sig, args.clone()) |
168 | } | 219 | } |
169 | Expr::MethodCall { receiver, args, .. } => { | 220 | Expr::MethodCall { receiver, args, .. } => { |
@@ -175,22 +226,25 @@ impl<'a, 'b> ExprValidator<'a, 'b> { | |||
175 | // if the receiver is of unknown type, it's very likely we | 226 | // if the receiver is of unknown type, it's very likely we |
176 | // don't know enough to correctly resolve the method call. | 227 | // don't know enough to correctly resolve the method call. |
177 | // This is kind of a band-aid for #6975. | 228 | // This is kind of a band-aid for #6975. |
178 | return None; | 229 | return; |
179 | } | 230 | } |
180 | 231 | ||
181 | // FIXME: note that we erase information about substs here. This | 232 | // FIXME: note that we erase information about substs here. This |
182 | // is not right, but, luckily, doesn't matter as we care only | 233 | // is not right, but, luckily, doesn't matter as we care only |
183 | // about the number of params | 234 | // about the number of params |
184 | let callee = self.infer.method_resolution(call_id)?; | 235 | let callee = match self.infer.method_resolution(call_id) { |
236 | Some(callee) => callee, | ||
237 | None => return, | ||
238 | }; | ||
185 | let sig = db.callable_item_signature(callee.into()).value; | 239 | let sig = db.callable_item_signature(callee.into()).value; |
186 | 240 | ||
187 | (sig, args) | 241 | (sig, args) |
188 | } | 242 | } |
189 | _ => return None, | 243 | _ => return, |
190 | }; | 244 | }; |
191 | 245 | ||
192 | if sig.is_varargs { | 246 | if sig.is_varargs { |
193 | return None; | 247 | return; |
194 | } | 248 | } |
195 | 249 | ||
196 | let params = sig.params(); | 250 | let params = sig.params(); |
@@ -213,8 +267,6 @@ impl<'a, 'b> ExprValidator<'a, 'b> { | |||
213 | }); | 267 | }); |
214 | } | 268 | } |
215 | } | 269 | } |
216 | |||
217 | None | ||
218 | } | 270 | } |
219 | 271 | ||
220 | fn validate_match( | 272 | fn validate_match( |
diff --git a/crates/ide/src/diagnostics.rs b/crates/ide/src/diagnostics.rs index b35bc2bae..8607139ba 100644 --- a/crates/ide/src/diagnostics.rs +++ b/crates/ide/src/diagnostics.rs | |||
@@ -136,6 +136,9 @@ pub(crate) fn diagnostics( | |||
136 | .on::<hir::diagnostics::IncorrectCase, _>(|d| { | 136 | .on::<hir::diagnostics::IncorrectCase, _>(|d| { |
137 | res.borrow_mut().push(warning_with_fix(d, &sema)); | 137 | res.borrow_mut().push(warning_with_fix(d, &sema)); |
138 | }) | 138 | }) |
139 | .on::<hir::diagnostics::ReplaceFilterMapNextWithFindMap, _>(|d| { | ||
140 | res.borrow_mut().push(warning_with_fix(d, &sema)); | ||
141 | }) | ||
139 | .on::<hir::diagnostics::InactiveCode, _>(|d| { | 142 | .on::<hir::diagnostics::InactiveCode, _>(|d| { |
140 | // If there's inactive code somewhere in a macro, don't propagate to the call-site. | 143 | // If there's inactive code somewhere in a macro, don't propagate to the call-site. |
141 | if d.display_source().file_id.expansion_info(db).is_some() { | 144 | if d.display_source().file_id.expansion_info(db).is_some() { |
diff --git a/crates/ide/src/diagnostics/fixes.rs b/crates/ide/src/diagnostics/fixes.rs index 579d5a308..eafbac43a 100644 --- a/crates/ide/src/diagnostics/fixes.rs +++ b/crates/ide/src/diagnostics/fixes.rs | |||
@@ -4,7 +4,7 @@ use hir::{ | |||
4 | db::AstDatabase, | 4 | db::AstDatabase, |
5 | diagnostics::{ | 5 | diagnostics::{ |
6 | Diagnostic, IncorrectCase, MissingFields, MissingOkOrSomeInTailExpr, NoSuchField, | 6 | Diagnostic, IncorrectCase, MissingFields, MissingOkOrSomeInTailExpr, NoSuchField, |
7 | RemoveThisSemicolon, UnresolvedModule, | 7 | RemoveThisSemicolon, ReplaceFilterMapNextWithFindMap, UnresolvedModule, |
8 | }, | 8 | }, |
9 | HasSource, HirDisplay, InFile, Semantics, VariantDef, | 9 | HasSource, HirDisplay, InFile, Semantics, VariantDef, |
10 | }; | 10 | }; |
@@ -144,6 +144,37 @@ impl DiagnosticWithFix for IncorrectCase { | |||
144 | } | 144 | } |
145 | } | 145 | } |
146 | 146 | ||
147 | // Bugs: | ||
148 | // * Action is applicable for both iter() and filter_map() rows | ||
149 | // * Action deletes the entire method chain | ||
150 | impl DiagnosticWithFix for ReplaceFilterMapNextWithFindMap { | ||
151 | fn fix(&self, sema: &Semantics<RootDatabase>) -> Option<Fix> { | ||
152 | let root = sema.db.parse_or_expand(self.file)?; | ||
153 | |||
154 | let next_expr = self.next_expr.to_node(&root); | ||
155 | let next_expr_range = next_expr.syntax().text_range(); | ||
156 | |||
157 | let filter_map_expr = self.filter_map_expr.to_node(&root); | ||
158 | let filter_map_expr_range = filter_map_expr.syntax().text_range(); | ||
159 | |||
160 | let edit = TextEdit::delete(next_expr_range); | ||
161 | |||
162 | // This is the entire method chain, including the array literal | ||
163 | eprintln!("NEXT EXPR: {:#?}", next_expr); | ||
164 | // This is the entire method chain except for the final next() | ||
165 | eprintln!("FILTER MAP EXPR: {:#?}", filter_map_expr); | ||
166 | |||
167 | let source_change = | ||
168 | SourceFileEdit { file_id: self.file.original_file(sema.db), edit }.into(); | ||
169 | |||
170 | Some(Fix::new( | ||
171 | "Replace filter_map(..).next() with find_map()", | ||
172 | source_change, | ||
173 | filter_map_expr_range, | ||
174 | )) | ||
175 | } | ||
176 | } | ||
177 | |||
147 | fn missing_record_expr_field_fix( | 178 | fn missing_record_expr_field_fix( |
148 | sema: &Semantics<RootDatabase>, | 179 | sema: &Semantics<RootDatabase>, |
149 | usage_file_id: FileId, | 180 | usage_file_id: FileId, |