Skip to content

const eval / trait deduction issues in nalgebra #11803

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.

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

Closed
digama0 opened this issue Mar 23, 2022 · 9 comments
Closed

const eval / trait deduction issues in nalgebra #11803

digama0 opened this issue Mar 23, 2022 · 9 comments
Labels
A-ty type system / type inference / traits / method resolution C-support Category: support questions

Comments

@digama0
Copy link
Contributor

digama0 commented Mar 23, 2022

The nalgebra crate has some nasty types that give rust-analyzer a hell of a time. I will try to make an example which does not depend on it, but for now here is a simple example depending on nalgebra 0.30.1:

use nalgebra::{Matrix4, Point3};

struct Foo;
impl From<Foo> for Point3<f32> {
  fn from(_: Foo) -> Self {
    Point3::from_slice(&[0.0, 0.0, 0.0])
  }
}

impl std::ops::Mul<Foo> for &Matrix4<f32> {
  type Output = Foo;
  fn mul(self, _: Foo) -> Self::Output { Foo }
}

fn test(mat: &Matrix4<f32>) -> Foo {
  let zero = Foo.into();
  let pt = mat.transform_point(&zero) - zero;
  let mat = mat.append_translation(&pt);
  let foo = &mat * Foo;
  foo
}

rustc can compile this just fine, but r-a gets confused and infers the types wrong, leading to an unresolved type at the end that causes a type error. Here are the types as resolved by rustc:

fn test(mat: &Matrix<f32, Const<4>, Const<4>, ArrayStorage<f32, 4, 4>>) -> Foo {
  let zero: OPoint<f32, Const<3>> = Foo.into();
  let pt: Matrix<f32, Const<3>, Const<1>, ArrayStorage<f32, 3, 1>> = mat.transform_point(&zero) - zero;
  let mat: Matrix<f32, Const<4>, Const<4>, ArrayStorage<f32, 4, 4>> = mat.append_translation(&pt);
  let foo: Foo = &mat * Foo;
  foo
}

and here are the types as resolved by rust-analyzer:

fn test(mat: &Matrix<f32, Const<3>, Const<3>, ArrayStorage<f32, 4, 4>>) -> Foo {
  let zero: OPoint<f32, Const<2>> = Foo.into();
  let pt: Matrix<f32, Const<2>, Const<1>, ArrayStorage<f32, 2, 1>> = mat.transform_point(&zero) - zero;
  let mat: Matrix<f32, Const<3>, Const<3>, ArrayStorage<f32, 3, 3>> = mat.append_translation(&pt);
  let foo: <&Matrix<f32, Const<3>, Const<3>, ArrayStorage<f32, 3, 3>> as Mul<Foo>>::Output = &mat * Foo;
  foo // type error
}

From what I can tell by commenting lines out:

  • With none of the code, mat is inferred to &Matrix<f32, Const<_>, Const<_>, ArrayStorage<f32, 4, 4>>. It is possible to resolve those numbers to 4 at this point, but perhaps it is not evaluating them because there is no need to at the moment.
  • The zero line on its own is of course unresolved, it requires the line after it.
  • For the pt line, the issue is that there are two impls of transform_point(), which look like this:
impl<T: RealField, S: Storage<T, Const<3>, Const<3>>> SquareMatrix<T, Const<3>, S> {
    pub fn transform_point(&self, pt: &Point<T, 2>) -> Point<T, 2> { ... }
}
impl<T: RealField, S: Storage<T, Const<4>, Const<4>>> SquareMatrix<T, Const<4>, S> {
    pub fn transform_point(&self, pt: &Point<T, 3>) -> Point<T, 3> { ... }
}
  • From what I can tell, it picks the first of these for reasons unknown, so now it knows that mat has type &Matrix<f32, Const<3>, Const<3>, ArrayStorage<f32, 4, 4>> (which is kind of nonsense but not ill formed).
  • The mat line then constructs a fully 3x3 matrix, presumably because of the type and const arguments on append_translation.
  • The multiplication is now clearly not resolved, since it yields an unreduced projection
  • and this leads to a type error when equating this projection type to Foo.

rust-analyzer version: c2ea378 2022-03-23 nightly

rustc version: rustc 1.59.0 (9d1b2106e 2022-02-23)

@Veykril Veykril added the A-ty type system / type inference / traits / method resolution label Mar 23, 2022
@flodiebold
Copy link
Member

flodiebold commented Mar 23, 2022

This might be #5441, or require some more const eval machinery. (See also #11772).

Actually, the "it picks the first of these for reasons unknown" is at least caused by the same root cause as #5441, though I don't know if fixing that will be enough.

@flodiebold flodiebold added the C-support Category: support questions label Mar 23, 2022
@digama0
Copy link
Contributor Author

digama0 commented Mar 23, 2022

I managed to make an nalgebra-free repro:

pub struct Const<const R: usize>;

impl Const<3> {
  pub fn foo(self) -> Const<2> { panic!() }
}

impl Const<4> {
  pub fn foo(self) -> Const<3> { panic!() }
}

struct Foo;
impl std::ops::Mul<Foo> for Const<3> {
  type Output = Foo;
  fn mul(self, _: Foo) -> Self::Output { Foo }
}

const FOUR: usize = 4;
fn test(c: Const<{ FOUR }>) -> Foo {
  let pt = c.foo();
  let foo = pt * Foo;
  foo
}

The fact that { FOUR } is a block seems to be critical; it causes rust-analyzer to defer evaluation of the expression, and then it is willing to consider the incorrect method. I think you are right that this has the same root cause as #5441.

@flodiebold
Copy link
Member

Well, my conclusion from this would actually be that it's a different root cause 😅 We'll see.

@HKalbasi
Copy link
Member

There is literaly no const eval in const generics yet, so Const<{ four }> is Const<_>. We can handle this special case easily, but the constants in nalgebra are very complex and evaluating them needs trait resolution (#4558) + lowering const generics to body (#7434) + evaluating assoc consts with generic parameters. Short term solution here IMO is replace type u4 = Const<{ typenum::U4::USIZE }> with type U4 = Const<4> in the nalgebra source code, which is an internal and invisible to user change.

@digama0
Copy link
Contributor Author

digama0 commented Mar 24, 2022

Considering that rust-analyzer already has {unknown} to avoid false positive errors like this one when the types and traits get too complicated, a possible solution here would be to resolve the function call and output type etc. to {unknown} if a method call is ambiguous due to unresolved constant evaluations. You wouldn't get any useful type information in the function, but at least it wouldn't give an error.

@flodiebold
Copy link
Member

Yes, in general it'd be good to avoid showing these kinds of "follow-up" errors. And we should detect ambiguous method resolutions instead of just choosing the first one.

@HKalbasi
Copy link
Member

@digama0 Btw, Is the original issue solved with the latest commit in nalgebra?

@digama0
Copy link
Contributor Author

digama0 commented Mar 24, 2022

Yes, the code example at the top works with that commit, as well as the real world example it was distilled from. So if you like you can close this issue, although of course the root cause is not fixed yet (but the linked issues could also be used to track that).

@flodiebold
Copy link
Member

Yes, let's close this then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ty type system / type inference / traits / method resolution C-support Category: support questions
Projects
None yet
Development

No branches or pull requests

4 participants