fix: Panic while hovering associated function with type annotation on generic param that not inherited from its container type

[ShoyuVanilla]

Fixes #17871

We call generic_args_sans_defaults here;

rust-analyzer/crates/hir-ty/src/display.rs

Lines 1021 to 1034 in 64a1405

	let parameters = parameters.as_slice(Interner);
	// We print all params except implicit impl Trait params. Still a bit weird; should we leave out parent and self?
	if parameters.len() - impl_ > 0 {
	// `parameters` are in the order of fn's params (including impl traits), fn's lifetimes
	let without_impl = self_param as usize + type_ + const_ + lifetime;
	// parent's params (those from enclosing impl or trait, if any).
	let (fn_params, parent_params) = parameters.split_at(without_impl + impl_);
	debug_assert_eq!(parent_params.len(), parent_len);
	let parent_params =
	generic_args_sans_defaults(f, Some(generic_def_id), parent_params);
	let fn_params =
	&generic_args_sans_defaults(f, Some(generic_def_id), fn_params)
	[0..without_impl];

but the following substitution inside that function panic in #17871;

rust-analyzer/crates/hir-ty/src/display.rs

Line 1468 in 64a1405

arg != &default_parameter.clone().substitute(Interner, &parameters)

it's because the Binders.binder inside default_parameters has a same length with the generics of the function we are hovering on, but the generics of it is split into two, fn_params and parent_params.
Because of this, it may panic if the function has one or more default parameters and both fn_params and parent_params are non-empty, like the case in the title of this PR.

So, we must call generic_args_sans_default first and then split it into fn_params and parent_params

[flodiebold]

@bors r+

[bors]

📌 Commit f4b7e81 has been approved by flodiebold

It is now in the queue for this repository.

[bors]

⌛ Testing commit f4b7e81 with merge 2b86639...

[bors]

☀️ Test successful - checks-actions
Approved by: flodiebold
Pushing 2b86639 to master...

[Veykril]

Hmm, this does look like the wrong way to render this doesn't it. Ideally we'd render this as fn <S as T>::f::<i32>() or so?

[ShoyuVanilla]

Yes, I think so, too. Currently we are rendering such container type into <> when rendering associated function

rust-analyzer 1.80.0 (05147895 2024-07-21)

And we can find such things in many places of our test cases like

rust-analyzer/crates/hir-ty/src/tests/traits.rs

Line 1108 in 2b86639

430..440 'foo::<u32>': fn foo<u32>(S) -> u32

I tried to fix them altogather when I was dealing with this issue, but it seems that our generic displaying has many problems that should be dealt with - especially when there are 'parent' generic parameters -, so I just turned to fix this panic with minimal touch and file the issue in proper way 🤔

[Veykril]

Yes, there was no need to do all of that in this PR given its separate issues

[ShoyuVanilla]

One thing I think that strange is;

We are rendering generics in many cases with fn hir_fmt_generic_arguments

rust-analyzer/crates/hir-ty/src/display.rs

Lines 1484 to 1518 in 2b86639

	fn hir_fmt_generic_arguments(
	f: &mut HirFormatter<'_>,
	parameters: &[GenericArg],
	self_: Option<&Ty>,
	) -> Result<(), HirDisplayError> {
	let mut first = true;
	let lifetime_offset = parameters.iter().position(|arg| arg.lifetime(Interner).is_some());
	let (ty_or_const, lifetimes) = match lifetime_offset {
	Some(offset) => parameters.split_at(offset),
	None => (parameters, &[][..]),
	};
	for generic_arg in lifetimes.iter().chain(ty_or_const) {
	// FIXME: Remove this
	// most of our lifetimes will be errors as we lack elision and inference
	// so don't render them for now
	if !cfg!(test)
	&& matches!(
	generic_arg.lifetime(Interner),
	Some(l) if ***l.interned() == LifetimeData::Error
	)
	{
	continue;
	}
	if !mem::take(&mut first) {
	write!(f, ", ")?;
	}
	match self_ {
	self_ @ Some(_) if generic_arg.ty(Interner) == self_ => write!(f, "Self")?,
	_ => generic_arg.hir_fmt(f)?,
	}
	}
	Ok(())
	}

and this moves lifetimes before the type and const parameters as it assumes that the input it gets has types and const parameters before the lifetime parameters, as generics.iter() gives them in this order;

rust-analyzer/crates/hir-ty/src/generics.rs

Lines 88 to 95 in 2b86639

	/// Iterate over the params without parent params.
	pub(crate) fn iter_self(
	&self,
	) -> impl DoubleEndedIterator<Item = (GenericParamId, GenericParamDataRef<'_>)> + '_ {
	let mut toc = self.params.iter_type_or_consts().map(from_toc_id(self));
	let trait_self_param = self.has_trait_self_param.then(|| toc.next()).flatten();
	chain!(trait_self_param, self.params.iter_lt().map(from_lt_id(self)), toc)
	}

But this function does not do such re-ordering to parent parameters and we only do such re-ordering manually in the block I've modified in this PR.

So, in other types the parent generics can be renderered in the rear position of the generic parameters and I'm not sure whether that's correct.

And If we should do such re-ordering everywhere we have to thinkabout grand-parent generics as well, I think 🤔

Furthermore, in fn generic_args_sans_defaults, we assume that the default parameters are comming in the consecutive last some parameters and this is correct in general in proper rust syntax(L1476)

rust-analyzer/crates/hir-ty/src/display.rs

Lines 1431 to 1482 in 2b86639

	fn generic_args_sans_defaults<'ga>(
	f: &mut HirFormatter<'_>,
	generic_def: Option<hir_def::GenericDefId>,
	parameters: &'ga [GenericArg],
	) -> &'ga [GenericArg] {
	if f.display_target.is_source_code() || f.omit_verbose_types() {
	match generic_def
	.map(|generic_def_id| f.db.generic_defaults(generic_def_id))
	.filter(|it| !it.is_empty())
	{
	None => parameters,
	Some(default_parameters) => {
	let should_show = |arg: &GenericArg, i: usize| {
	let is_err = |arg: &GenericArg| match arg.data(Interner) {
	chalk_ir::GenericArgData::Lifetime(it) => {
	*it.data(Interner) == LifetimeData::Error
	}
	chalk_ir::GenericArgData::Ty(it) => *it.kind(Interner) == TyKind::Error,
	chalk_ir::GenericArgData::Const(it) => matches!(
	it.data(Interner).value,
	ConstValue::Concrete(ConcreteConst {
	interned: ConstScalar::Unknown,
	..
	})
	),
	};
	// if the arg is error like, render it to inform the user
	if is_err(arg) {
	return true;
	}
	// otherwise, if the arg is equal to the param default, hide it (unless the
	// default is an error which can happen for the trait Self type)
	#[allow(unstable_name_collisions)]
	default_parameters.get(i).is_none_or(|default_parameter| {
	// !is_err(default_parameter.skip_binders())
	// &&
	arg != &default_parameter.clone().substitute(Interner, &parameters)
	})
	};
	let mut default_from = 0;
	for (i, parameter) in parameters.iter().enumerate() {
	if should_show(parameter, i) {
	default_from = i + 1;
	}
	}
	&parameters[0..default_from]
	}
	}
	} else {
	parameters
	}
	}

But I think that this can be problematic if we have both non-empty self parameters and parent parameters, and they are have some both defaults and non defaults.
In this case, aren't we losing some non-default generic parameter of the parent? 🤔

[ShoyuVanilla]

So, I'm just trying to specify the exact problems we have here with some experiments 😅

[Veykril]

That might be leftovers, we used to have lifetimes after type/const params, but we've since fixed that order. (this is also why some stuff might look a bit weird still with lifetime param handling)

grand-parent generics as well, I think

I don't think Rust currently has that anywhere?

[Veykril]

In general the order ought to be

rust-analyzer/crates/hir-ty/src/generics.rs

Lines 1 to 9 in bd4785a

	//! Utilities for working with generics.
	//!
	//! The layout for generics as expected by chalk are as follows:
	//! - Optional Self parameter
	//! - Lifetime parameters
	//! - Type or Const parameters
	//! - Parent parameters
	//!
	//! where parent follows the same scheme.

now. Ideally parents should come first though I think, but that doesn't work due to chalk iirc