Skip to content
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

nalgebra depends on incomplete guidance from env during method selection #161

Closed
lcnr opened this issue Feb 11, 2025 · 1 comment · Fixed by rust-lang/rust#136928
Closed
Assignees

Comments

@lcnr
Copy link
Contributor

lcnr commented Feb 11, 2025

the following snippet is ambig with the new solver and compiles with old

use num_traits::Zero;
use nalgebra::{DimName, Scalar, U1, Matrix};
use nalgebra::DefaultAllocator;
use nalgebra::allocator::Allocator;

type Alias<T, D> = Matrix<T, D, U1, <DefaultAllocator as Allocator<D>>::Buffer<T>>;
fn foo<T: Scalar + Zero, D: DimName>() -> Matrix<T, D, U1, <DefaultAllocator as Allocator<D>>::Buffer<T>>
where
    DefaultAllocator: Allocator<D>,
{
    Matrix::<_, _, U1, <DefaultAllocator as Allocator<_>>::Buffer<T>>::from_element(T::zero()) // works
    // Alias::<T, _>::from_element(T::zero()) // error
}

minimizing this to not depend on nalgebra results in the following

trait Constrain<T> {
    type Assoc;
}
impl<T> Constrain<T> for () {
    type Assoc = ();
}
struct Foo<T, U = <() as Constrain<T>>::Assoc>(T, U);

impl<T: Copy> Foo<T> {
    fn foo() {}
}
struct B;
impl Foo<B> {
    fn foo() {}
}

type Alias<T> = Foo<T>;
fn via_guidance<T: Copy>()
where
    (): Constrain<T>,
{
    Alias::foo();
}

this errors with the following with the new solver

error[E0034]: multiple applicable items in scope
  --> src/lib.rs:26:10
   |
26 |     Foo::foo();
   |          ^^^ multiple `foo` found
   |
note: candidate #1 is defined in an impl for the type `Foo<B>`
  --> src/lib.rs:17:5
   |
17 |     fn foo() {}
   |     ^^^^^^^^
note: candidate #2 is defined in an impl for the type `Foo<T>`
  --> src/lib.rs:13:5
   |
13 |     fn foo() {}
   |     ^^^^^^^^

This compiles in the old solver as Alias expands to Foo<?x, <() as Constrain<?x>>::Assoc>. Normalizing <() as Constrain<?x>>::Assoc constrains ?x to T, at which point method selection has a unique candidate.

In the new solver we never try to normalize <() as Constrain<?x>>::Assoc, causing ?x to remain ambiguous at this point, resulting in an ambiguity error during method selection.

Subtle: using Foo instead of Alias errors on stable as well, as it gets lowered to Foo<?x, ?y> instead without using the default argument. Foo::<_> also gets lowered to Foo<?x, ?y>. We only use default arguments if infer_args is set to false during ast lowering in the lowerer. I don't understand why that is the case tbh 🤷

@lcnr
Copy link
Contributor Author

lcnr commented Feb 11, 2025

fixed by adding the following code 💀 the alternative of forcing nalgebra to stop relying on this behavior seems bad, esp as we can't really future compat lint this in the old solver 🤔

diff --git a/compiler/rustc_hir_typeck/src/method/mod.rs b/compiler/rustc_hir_typeck/src/method/mod.rs
index 4008021c3a8..ceadb8a5c12 100644
--- a/compiler/rustc_hir_typeck/src/method/mod.rs
+++ b/compiler/rustc_hir_typeck/src/method/mod.rs
@@ -494,6 +494,14 @@ pub(crate) fn resolve_fully_qualified_call(
             }
         }
 
+        // HACK: TODO
+        let self_ty = {
+            let infer = self.infcx.next_ty_var(span);
+            self.demand_eqtype(span, infer, self_ty);
+            self.select_obligations_where_possible(|_| {});
+            self.resolve_vars_if_possible(infer)
+        };
+
         let pick = self.probe_for_name(
             probe::Mode::Path,
             method_name,

@lcnr lcnr moved this to in progress in -Znext-solver=globally Feb 12, 2025
@lcnr lcnr self-assigned this Feb 13, 2025
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Feb 14, 2025
…piler-errors

eagerly prove WF when resolving fully qualified paths

fixes rust-lang/trait-system-refactor-initiative#161.

This hopefully shouldn't impact perf. I do think we need to deal with at least part of the fallout here, opening for vibes.

r? `@compiler-errors`
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Feb 14, 2025
Rollup merge of rust-lang#136928 - lcnr:method-lookup-check-wf, r=compiler-errors

eagerly prove WF when resolving fully qualified paths

fixes rust-lang/trait-system-refactor-initiative#161.

This hopefully shouldn't impact perf. I do think we need to deal with at least part of the fallout here, opening for vibes.

r? ``@compiler-errors``
@lcnr lcnr moved this from in progress to done in -Znext-solver=globally Feb 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging a pull request may close this issue.

1 participant