Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 15, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 7 additions & 8 deletions crates/hir-ty/src/display.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1019,26 +1019,25 @@ impl HirDisplay for Ty {
let (parent_len, self_param, type_, const_, impl_, lifetime) =
generics.provenance_split();
let parameters = parameters.as_slice(Interner);
debug_assert_eq!(
parameters.len(),
parent_len + self_param as usize + type_ + const_ + impl_ + lifetime
);
// 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 parameters =
generic_args_sans_defaults(f, Some(generic_def_id), parameters);
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];

write!(f, "<")?;
hir_fmt_generic_arguments(f, parent_params, None)?;
if !parent_params.is_empty() && !fn_params.is_empty() {
write!(f, ", ")?;
}
hir_fmt_generic_arguments(f, fn_params, None)?;
hir_fmt_generic_arguments(f, &fn_params[0..without_impl], None)?;
write!(f, ">")?;
}
}
Expand Down
28 changes: 28 additions & 0 deletions crates/ide/src/hover/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8602,3 +8602,31 @@ fn test() {
"#]],
);
}

#[test]
fn issue_17871() {
check(
r#"
trait T {
fn f<A>();
}

struct S {}
impl T for S {
fn f<A>() {}
}

fn main() {
let x$0 = S::f::<i32>;
}
"#,
expect![[r#"
*x*

```rust
// size = 0, align = 1
let x: fn f<S, i32>()
Copy link
Member

@Veykril Veykril Aug 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@ShoyuVanilla ShoyuVanilla Aug 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
image

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 🤔

Copy link
Member

@Veykril Veykril Aug 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@ShoyuVanilla ShoyuVanilla Aug 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 🤔

Copy link
Member Author

@ShoyuVanilla ShoyuVanilla Aug 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@Veykril Veykril Aug 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@Veykril Veykril Aug 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

```
"#]],
);
}