From 2b382eb7722272759f0ddea27dc788140986a859 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Mon, 31 Mar 2025 07:43:08 +0200 Subject: [PATCH] fix: Cleanup param name inlay hint filtering --- crates/hir/src/attrs.rs | 2 +- crates/hir/src/lib.rs | 16 +- .../src/handlers/extract_function.rs | 2 +- crates/ide-db/src/syntax_helpers/node_ext.rs | 4 +- crates/ide/src/inlay_hints/generic_param.rs | 49 ++-- crates/ide/src/inlay_hints/param_name.rs | 224 +++++++++++------- crates/syntax/src/ast/node_ext.rs | 10 + 7 files changed, 188 insertions(+), 119 deletions(-) diff --git a/crates/hir/src/attrs.rs b/crates/hir/src/attrs.rs index 70637028ef..e71b51bfa4 100644 --- a/crates/hir/src/attrs.rs +++ b/crates/hir/src/attrs.rs @@ -260,7 +260,7 @@ fn resolve_impl_trait_item( // attributes here. Use path resolution directly instead. // // FIXME: resolve type aliases (which are not yielded by iterate_path_candidates) - method_resolution::iterate_path_candidates( + _ = method_resolution::iterate_path_candidates( &canonical, db, environment, diff --git a/crates/hir/src/lib.rs b/crates/hir/src/lib.rs index c5ed044683..7f0382fa93 100644 --- a/crates/hir/src/lib.rs +++ b/crates/hir/src/lib.rs @@ -2922,10 +2922,14 @@ impl Trait { db: &dyn HirDatabase, ) -> Option> { let mut violations = vec![]; - hir_ty::dyn_compatibility::dyn_compatibility_with_callback(db, self.id, &mut |violation| { - violations.push(violation); - ControlFlow::Continue(()) - }); + _ = hir_ty::dyn_compatibility::dyn_compatibility_with_callback( + db, + self.id, + &mut |violation| { + violations.push(violation); + ControlFlow::Continue(()) + }, + ); violations.is_empty().not().then_some(violations) } @@ -5514,7 +5518,7 @@ impl Type { .generic_def() .map_or_else(|| TraitEnvironment::empty(krate.id), |d| db.trait_environment(d)); - method_resolution::iterate_method_candidates_dyn( + _ = method_resolution::iterate_method_candidates_dyn( &canonical, db, environment, @@ -5601,7 +5605,7 @@ impl Type { .generic_def() .map_or_else(|| TraitEnvironment::empty(krate.id), |d| db.trait_environment(d)); - method_resolution::iterate_path_candidates( + _ = method_resolution::iterate_path_candidates( &canonical, db, environment, diff --git a/crates/ide-assists/src/handlers/extract_function.rs b/crates/ide-assists/src/handlers/extract_function.rs index abc0698b01..5a6a7ede35 100644 --- a/crates/ide-assists/src/handlers/extract_function.rs +++ b/crates/ide-assists/src/handlers/extract_function.rs @@ -751,7 +751,7 @@ impl FunctionBody { ast::Stmt::Item(_) => (), ast::Stmt::LetStmt(stmt) => { if let Some(pat) = stmt.pat() { - walk_pat(&pat, &mut |pat| { + _ = walk_pat(&pat, &mut |pat| { cb(pat); std::ops::ControlFlow::<(), ()>::Continue(()) }); diff --git a/crates/ide-db/src/syntax_helpers/node_ext.rs b/crates/ide-db/src/syntax_helpers/node_ext.rs index cd47524caa..bdff64dd08 100644 --- a/crates/ide-db/src/syntax_helpers/node_ext.rs +++ b/crates/ide-db/src/syntax_helpers/node_ext.rs @@ -121,7 +121,7 @@ pub fn walk_patterns_in_expr(start: &ast::Expr, cb: &mut dyn FnMut(ast::Pat)) { match ast::Stmt::cast(node.clone()) { Some(ast::Stmt::LetStmt(l)) => { if let Some(pat) = l.pat() { - walk_pat(&pat, &mut |pat| { + _ = walk_pat(&pat, &mut |pat| { cb(pat); ControlFlow::<(), ()>::Continue(()) }); @@ -159,7 +159,7 @@ pub fn walk_patterns_in_expr(start: &ast::Expr, cb: &mut dyn FnMut(ast::Pat)) { } } else if let Some(pat) = ast::Pat::cast(node) { preorder.skip_subtree(); - walk_pat(&pat, &mut |pat| { + _ = walk_pat(&pat, &mut |pat| { cb(pat); ControlFlow::<(), ()>::Continue(()) }); diff --git a/crates/ide/src/inlay_hints/generic_param.rs b/crates/ide/src/inlay_hints/generic_param.rs index fc1083fdca..730732df86 100644 --- a/crates/ide/src/inlay_hints/generic_param.rs +++ b/crates/ide/src/inlay_hints/generic_param.rs @@ -1,4 +1,5 @@ //! Implementation of inlay hints for generic parameters. +use either::Either; use ide_db::{active_parameter::generic_def_for_node, famous_defs::FamousDefs}; use syntax::{ AstNode, @@ -6,7 +7,8 @@ use syntax::{ }; use crate::{ - InlayHint, InlayHintLabel, InlayHintsConfig, InlayKind, inlay_hints::GenericParameterHints, + InlayHint, InlayHintLabel, InlayHintsConfig, InlayKind, + inlay_hints::{GenericParameterHints, param_name}, }; use super::param_name::is_argument_similar_to_param_name; @@ -62,8 +64,17 @@ pub(crate) fn hints( let param_name = param.name(sema.db); let should_hide = { - let argument = get_string_representation(&arg)?; - is_argument_similar_to_param_name(&argument, param_name.as_str()) + let param_name = param_name.as_str(); + get_segment_representation(&arg).map_or(false, |seg| match seg { + Either::Left(Either::Left(argument)) => { + is_argument_similar_to_param_name(&argument, param_name) + } + Either::Left(Either::Right(argument)) => argument + .segment() + .and_then(|it| it.name_ref()) + .is_some_and(|it| it.text().eq_ignore_ascii_case(param_name)), + Either::Right(lifetime) => lifetime.text().eq_ignore_ascii_case(param_name), + }) }; if should_hide { @@ -111,32 +122,34 @@ pub(crate) fn hints( Some(()) } -fn get_string_representation(arg: &ast::GenericArg) -> Option { +fn get_segment_representation( + arg: &ast::GenericArg, +) -> Option, ast::Path>, ast::Lifetime>> { return match arg { ast::GenericArg::AssocTypeArg(_) => None, - ast::GenericArg::ConstArg(const_arg) => Some(const_arg.to_string()), + ast::GenericArg::ConstArg(const_arg) => { + param_name::get_segment_representation(&const_arg.expr()?).map(Either::Left) + } ast::GenericArg::LifetimeArg(lifetime_arg) => { let lifetime = lifetime_arg.lifetime()?; - Some(lifetime.to_string()) + Some(Either::Right(lifetime)) } ast::GenericArg::TypeArg(type_arg) => { let ty = type_arg.ty()?; - Some( - type_path_segment(&ty) - .map_or_else(|| type_arg.to_string(), |segment| segment.to_string()), - ) + type_path(&ty).map(Either::Right).map(Either::Left) } }; - fn type_path_segment(ty: &ast::Type) -> Option { + fn type_path(ty: &ast::Type) -> Option { match ty { - ast::Type::ArrayType(it) => type_path_segment(&it.ty()?), - ast::Type::ForType(it) => type_path_segment(&it.ty()?), - ast::Type::ParenType(it) => type_path_segment(&it.ty()?), - ast::Type::PathType(path_type) => path_type.path()?.segment(), - ast::Type::PtrType(it) => type_path_segment(&it.ty()?), - ast::Type::RefType(it) => type_path_segment(&it.ty()?), - ast::Type::SliceType(it) => type_path_segment(&it.ty()?), + ast::Type::ArrayType(it) => type_path(&it.ty()?), + ast::Type::ForType(it) => type_path(&it.ty()?), + ast::Type::ParenType(it) => type_path(&it.ty()?), + ast::Type::PathType(path_type) => path_type.path(), + ast::Type::PtrType(it) => type_path(&it.ty()?), + ast::Type::RefType(it) => type_path(&it.ty()?), + ast::Type::SliceType(it) => type_path(&it.ty()?), + ast::Type::MacroType(macro_type) => macro_type.macro_call()?.path(), _ => None, } } diff --git a/crates/ide/src/inlay_hints/param_name.rs b/crates/ide/src/inlay_hints/param_name.rs index 44ea5351fb..99c698ce02 100644 --- a/crates/ide/src/inlay_hints/param_name.rs +++ b/crates/ide/src/inlay_hints/param_name.rs @@ -4,16 +4,15 @@ //! _ = max(/*x*/4, /*y*/4); //! ``` +use std::iter::zip; + use either::Either; -use hir::{Callable, Semantics}; +use hir::Semantics; use ide_db::{RootDatabase, famous_defs::FamousDefs}; use span::EditionedFileId; use stdx::to_lower_snake_case; -use syntax::{ - ToSmolStr, - ast::{self, AstNode, HasArgList, HasName, UnaryOp}, -}; +use syntax::ast::{self, AstNode, HasArgList, HasName, UnaryOp}; use crate::{InlayHint, InlayHintLabel, InlayHintPosition, InlayHintsConfig, InlayKind}; @@ -29,6 +28,12 @@ pub(super) fn hints( } let (callable, arg_list) = get_callable(sema, &expr)?; + let unary_function = callable.n_params() == 1; + let function_name = match callable.kind() { + hir::CallableKind::Function(function) => Some(function.name(sema.db)), + _ => None, + }; + let function_name = function_name.as_ref().map(|it| it.as_str()); let hints = callable .params() .into_iter() @@ -40,7 +45,13 @@ pub(super) fn hints( Some((p, param_name, arg, range)) }) .filter(|(_, param_name, arg, _)| { - !should_hide_param_name_hint(sema, &callable, param_name.as_str(), arg) + !should_hide_param_name_hint( + sema, + unary_function, + function_name, + param_name.as_str(), + arg, + ) }) .map(|(param, param_name, _, hir::FileRange { range, .. })| { let colon = if config.render_colons { ":" } else { "" }; @@ -94,9 +105,13 @@ fn get_callable( } } +const INSIGNIFICANT_METHOD_NAMES: &[&str] = &["clone", "as_ref", "into"]; +const INSIGNIFICANT_PARAMETER_NAMES: &[&str] = &["predicate", "value", "pat", "rhs", "other"]; + fn should_hide_param_name_hint( sema: &Semantics<'_, RootDatabase>, - callable: &hir::Callable, + unary_function: bool, + function_name: Option<&str>, param_name: &str, argument: &ast::Expr, ) -> bool { @@ -114,95 +129,128 @@ fn should_hide_param_name_hint( return true; } - if matches!(argument, ast::Expr::PrefixExpr(prefix) if prefix.op_kind() == Some(UnaryOp::Not)) { - return false; + if param_name.starts_with("ra_fixture") { + return true; } - let fn_name = match callable.kind() { - hir::CallableKind::Function(it) => Some(it.name(sema.db).as_str().to_smolstr()), - _ => None, - }; - let fn_name = fn_name.as_deref(); - is_param_name_suffix_of_fn_name(param_name, callable, fn_name) - || is_argument_expr_similar_to_param_name(argument, param_name) - || param_name.starts_with("ra_fixture") - || (callable.n_params() == 1 && is_obvious_param(param_name)) - || is_adt_constructor_similar_to_param_name(sema, argument, param_name) + if unary_function { + if let Some(function_name) = function_name { + if is_param_name_suffix_of_fn_name(param_name, function_name) { + return true; + } + } + if is_obvious_param(param_name) { + return true; + } + } + + is_argument_expr_similar_to_param_name(sema, argument, param_name) } /// Hide the parameter name of a unary function if it is a `_` - prefixed suffix of the function's name, or equal. /// /// `fn strip_suffix(suffix)` will be hidden. /// `fn stripsuffix(suffix)` will not be hidden. -fn is_param_name_suffix_of_fn_name( - param_name: &str, - callable: &Callable, - fn_name: Option<&str>, -) -> bool { - match (callable.n_params(), fn_name) { - (1, Some(function)) => { - function == param_name - || function - .len() - .checked_sub(param_name.len()) - .and_then(|at| function.is_char_boundary(at).then(|| function.split_at(at))) - .is_some_and(|(prefix, suffix)| { - suffix.eq_ignore_ascii_case(param_name) && prefix.ends_with('_') - }) - } - _ => false, - } +fn is_param_name_suffix_of_fn_name(param_name: &str, fn_name: &str) -> bool { + fn_name == param_name + || fn_name + .len() + .checked_sub(param_name.len()) + .and_then(|at| fn_name.is_char_boundary(at).then(|| fn_name.split_at(at))) + .is_some_and(|(prefix, suffix)| { + suffix.eq_ignore_ascii_case(param_name) && prefix.ends_with('_') + }) } -fn is_argument_expr_similar_to_param_name(argument: &ast::Expr, param_name: &str) -> bool { - let argument = match get_string_representation(argument) { - Some(argument) => argument, - None => return false, - }; - is_argument_similar_to_param_name(&argument, param_name) +fn is_argument_expr_similar_to_param_name( + sema: &Semantics<'_, RootDatabase>, + argument: &ast::Expr, + param_name: &str, +) -> bool { + match get_segment_representation(argument) { + Some(Either::Left(argument)) => is_argument_similar_to_param_name(&argument, param_name), + Some(Either::Right(path)) => { + path.segment() + .and_then(|it| it.name_ref()) + .is_some_and(|name_ref| name_ref.text().eq_ignore_ascii_case(param_name)) + || is_adt_constructor_similar_to_param_name(sema, &path, param_name) + } + None => false, + } } /// Check whether param_name and argument are the same or /// whether param_name is a prefix/suffix of argument(split at `_`). -pub(super) fn is_argument_similar_to_param_name(argument: &str, param_name: &str) -> bool { - // std is honestly too panic happy... - let str_split_at = |str: &str, at| str.is_char_boundary(at).then(|| argument.split_at(at)); +pub(super) fn is_argument_similar_to_param_name( + argument: &[ast::NameRef], + param_name: &str, +) -> bool { + debug_assert!(!argument.is_empty()); + debug_assert!(!param_name.is_empty()); + let param_name = param_name.split('_'); + let argument = argument.iter().flat_map(|it| it.text_non_mutable().split('_')); - let param_name = param_name.trim_start_matches('_'); - let argument = argument.trim_start_matches('_'); - - match str_split_at(argument, param_name.len()) { - Some((prefix, rest)) if prefix.eq_ignore_ascii_case(param_name) => { - return rest.is_empty() || rest.starts_with('_'); - } - _ => (), - } - match argument.len().checked_sub(param_name.len()).and_then(|at| str_split_at(argument, at)) { - Some((rest, suffix)) if param_name.eq_ignore_ascii_case(suffix) => { - return rest.is_empty() || rest.ends_with('_'); - } - _ => (), - } - false + let prefix_match = zip(argument.clone(), param_name.clone()) + .all(|(arg, param)| arg.eq_ignore_ascii_case(param)); + let postfix_match = || { + zip(argument.rev(), param_name.rev()).all(|(arg, param)| arg.eq_ignore_ascii_case(param)) + }; + prefix_match || postfix_match() } -fn get_string_representation(expr: &ast::Expr) -> Option { +pub(super) fn get_segment_representation( + expr: &ast::Expr, +) -> Option, ast::Path>> { match expr { ast::Expr::MethodCallExpr(method_call_expr) => { + let receiver = + method_call_expr.receiver().and_then(|expr| get_segment_representation(&expr)); let name_ref = method_call_expr.name_ref()?; - match name_ref.text().as_str() { - "clone" | "as_ref" => method_call_expr.receiver().map(|rec| rec.to_string()), - name_ref => Some(name_ref.to_owned()), + if INSIGNIFICANT_METHOD_NAMES.contains(&name_ref.text().as_str()) { + return receiver; } + Some(Either::Left(match receiver { + Some(Either::Left(mut left)) => { + left.push(name_ref); + left + } + Some(Either::Right(_)) | None => vec![name_ref], + })) } - ast::Expr::MacroExpr(macro_expr) => { - Some(macro_expr.macro_call()?.path()?.segment()?.to_string()) + ast::Expr::FieldExpr(field_expr) => { + let expr = field_expr.expr().and_then(|expr| get_segment_representation(&expr)); + let name_ref = field_expr.name_ref()?; + let res = match expr { + Some(Either::Left(mut left)) => { + left.push(name_ref); + left + } + Some(Either::Right(_)) | None => vec![name_ref], + }; + Some(Either::Left(res)) } - ast::Expr::FieldExpr(field_expr) => Some(field_expr.name_ref()?.to_string()), - ast::Expr::PathExpr(path_expr) => Some(path_expr.path()?.segment()?.to_string()), - ast::Expr::PrefixExpr(prefix_expr) => get_string_representation(&prefix_expr.expr()?), - ast::Expr::RefExpr(ref_expr) => get_string_representation(&ref_expr.expr()?), - ast::Expr::CastExpr(cast_expr) => get_string_representation(&cast_expr.expr()?), + // paths + ast::Expr::MacroExpr(macro_expr) => macro_expr.macro_call()?.path().map(Either::Right), + ast::Expr::RecordExpr(record_expr) => record_expr.path().map(Either::Right), + ast::Expr::PathExpr(path_expr) => { + let path = path_expr.path()?; + // single segment paths are likely locals + Some(match path.as_single_name_ref() { + None => Either::Right(path), + Some(name_ref) => Either::Left(vec![name_ref]), + }) + } + ast::Expr::PrefixExpr(prefix_expr) if prefix_expr.op_kind() == Some(UnaryOp::Not) => None, + // recurse + ast::Expr::PrefixExpr(prefix_expr) => get_segment_representation(&prefix_expr.expr()?), + ast::Expr::RefExpr(ref_expr) => get_segment_representation(&ref_expr.expr()?), + ast::Expr::CastExpr(cast_expr) => get_segment_representation(&cast_expr.expr()?), + ast::Expr::CallExpr(call_expr) => get_segment_representation(&call_expr.expr()?), + ast::Expr::AwaitExpr(await_expr) => get_segment_representation(&await_expr.expr()?), + ast::Expr::IndexExpr(index_expr) => get_segment_representation(&index_expr.base()?), + ast::Expr::ParenExpr(paren_expr) => get_segment_representation(&paren_expr.expr()?), + ast::Expr::TryExpr(try_expr) => get_segment_representation(&try_expr.expr()?), + // ast::Expr::ClosureExpr(closure_expr) => todo!(), _ => None, } } @@ -210,30 +258,15 @@ fn get_string_representation(expr: &ast::Expr) -> Option { fn is_obvious_param(param_name: &str) -> bool { // avoid displaying hints for common functions like map, filter, etc. // or other obvious words used in std - let is_obvious_param_name = - matches!(param_name, "predicate" | "value" | "pat" | "rhs" | "other"); - param_name.len() == 1 || is_obvious_param_name + param_name.len() == 1 || INSIGNIFICANT_PARAMETER_NAMES.contains(¶m_name) } fn is_adt_constructor_similar_to_param_name( sema: &Semantics<'_, RootDatabase>, - argument: &ast::Expr, + path: &ast::Path, param_name: &str, ) -> bool { - let path = match argument { - ast::Expr::CallExpr(c) => c.expr().and_then(|e| match e { - ast::Expr::PathExpr(p) => p.path(), - _ => None, - }), - ast::Expr::PathExpr(p) => p.path(), - ast::Expr::RecordExpr(r) => r.path(), - _ => return false, - }; - let path = match path { - Some(it) => it, - None => return false, - }; - (|| match sema.resolve_path(&path)? { + (|| match sema.resolve_path(path)? { hir::PathResolution::Def(hir::ModuleDef::Adt(_)) => { Some(to_lower_snake_case(&path.segment()?.name_ref()?.text()) == param_name) } @@ -501,6 +534,7 @@ fn enum_matches_param_name(completion_kind: CompletionKind) {} fn foo(param: u32) {} fn bar(param_eter: u32) {} +fn baz(a_d_e: u32) {} enum CompletionKind { Keyword, @@ -553,6 +587,14 @@ fn main() { //^^^^^^^^^^^ param_eter non_ident_pat((0, 0)); + + baz(a.d.e); + baz(a.dc.e); + // ^^^^^^ a_d_e + baz(ac.d.e); + // ^^^^^^ a_d_e + baz(a.d.ec); + // ^^^^^^ a_d_e }"#, ); } diff --git a/crates/syntax/src/ast/node_ext.rs b/crates/syntax/src/ast/node_ext.rs index a5ac59dd7d..b9ccd34cff 100644 --- a/crates/syntax/src/ast/node_ext.rs +++ b/crates/syntax/src/ast/node_ext.rs @@ -36,6 +36,16 @@ impl ast::NameRef { pub fn text(&self) -> TokenText<'_> { text_of_first_token(self.syntax()) } + pub fn text_non_mutable(&self) -> &str { + fn first_token(green_ref: &GreenNodeData) -> &GreenTokenData { + green_ref.children().next().and_then(NodeOrToken::into_token).unwrap() + } + + match self.syntax().green() { + Cow::Borrowed(green_ref) => first_token(green_ref).text(), + Cow::Owned(_) => unreachable!(), + } + } pub fn as_tuple_field(&self) -> Option { self.text().parse().ok()