Fixes for consts

crates/hir/src/lib.rs

@@ -1541,9 +1541,7 @@ impl SelfParam {
 
 impl HasVisibility for Function {
     fn visibility(&self, db: &dyn HirDatabase) -> Visibility {
-        let function_data = db.function_data(self.id);
+        db.function_visibility(self.id)
-        let visibility = &function_data.visibility;
-        visibility.resolve(db.upcast(), &self.id.resolver(db.upcast()))
     }
 }
 
@@ -1594,9 +1592,7 @@ impl Const {
 
 impl HasVisibility for Const {
     fn visibility(&self, db: &dyn HirDatabase) -> Visibility {
-        let function_data = db.const_data(self.id);
+        db.const_visibility(self.id)
-        let visibility = &function_data.visibility;
-        visibility.resolve(db.upcast(), &self.id.resolver(db.upcast()))
     }
 }
 

crates/hir_def/src/db.rs

@@ -175,9 +175,13 @@ pub trait DefDatabase: InternDatabase + AstDatabase + Upcast<dyn AstDatabase> {
     #[salsa::invoke(visibility::field_visibilities_query)]
     fn field_visibilities(&self, var: VariantId) -> Arc<ArenaMap<LocalFieldId, Visibility>>;
 
+    // FIXME: unify function_visibility and const_visibility?
     #[salsa::invoke(visibility::function_visibility_query)]
     fn function_visibility(&self, def: FunctionId) -> Visibility;
 
+    #[salsa::invoke(visibility::const_visibility_query)]
+    fn const_visibility(&self, def: ConstId) -> Visibility;
+
     #[salsa::transparent]
     fn crate_limits(&self, crate_id: CrateId) -> CrateLimits;
 }

crates/hir_def/src/visibility.rs

@@ -11,7 +11,7 @@ use crate::{
     nameres::DefMap,
     path::{ModPath, PathKind},
     resolver::HasResolver,
-    FunctionId, HasModule, LocalFieldId, ModuleId, VariantId,
+    ConstId, FunctionId, HasModule, LocalFieldId, ModuleId, VariantId,
 };
 
 /// Visibility of an item, not yet resolved.
@@ -234,3 +234,9 @@ pub(crate) fn function_visibility_query(db: &dyn DefDatabase, def: FunctionId) -
     let resolver = def.resolver(db);
     db.function_data(def).visibility.resolve(db, &resolver)
 }
+
+/// Resolve visibility of a const.
+pub(crate) fn const_visibility_query(db: &dyn DefDatabase, def: ConstId) -> Visibility {
+    let resolver = def.resolver(db);
+    db.const_data(def).visibility.resolve(db, &resolver)
+}

crates/hir_ty/src/infer/unify.rs

@@ -379,6 +379,13 @@ impl<'a> InferenceTable<'a> {
         self.pending_obligations = snapshot.pending_obligations;
     }
 
+    pub(crate) fn run_in_snapshot<T>(&mut self, f: impl FnOnce(&mut InferenceTable) -> T) -> T {
+        let snapshot = self.snapshot();
+        let result = f(self);
+        self.rollback_to(snapshot);
+        result
+    }
+
     /// Checks an obligation without registering it. Useful mostly to check
     /// whether a trait *might* be implemented before deciding to 'lock in' the
     /// choice (during e.g. method resolution or deref).

crates/hir_ty/src/method_resolution.rs

@@ -985,7 +985,7 @@ fn is_valid_candidate(
                     return false;
                 }
             }
-            let snap = table.snapshot();
+            table.run_in_snapshot(|table| {
                 let subst = TyBuilder::subst_for_def(db, m).fill_with_inference_vars(table).build();
                 let expected_self_ty = match m.lookup(db.upcast()).container {
                     ItemContainerId::TraitId(_) => {
@@ -995,16 +995,15 @@ fn is_valid_candidate(
                         subst.apply(db.impl_self_ty(impl_id).skip_binders().clone(), Interner)
                     }
                     // We should only get called for associated items (impl/trait)
-                ItemContainerId::ModuleId(_) | ItemContainerId::ExternBlockId(_) => unreachable!(),
+                    ItemContainerId::ModuleId(_) | ItemContainerId::ExternBlockId(_) => {
+                        unreachable!()
+                    }
                 };
                 if !table.unify(&expected_self_ty, &self_ty) {
-                // FIXME handle rollbacks better
-                table.rollback_to(snap);
                     return false;
                 }
                 if let Some(receiver_ty) = receiver_ty {
                     if !data.has_self_param() {
-                    table.rollback_to(snap);
                         return false;
                     }
 
@@ -1012,13 +1011,10 @@ fn is_valid_candidate(
                     let expected_receiver =
                         sig.map(|s| s.params()[0].clone()).substitute(Interner, &subst);
                     let receiver_matches = table.unify(&receiver_ty, &expected_receiver);
-                table.rollback_to(snap);
 
                     if !receiver_matches {
                         return false;
                     }
-            } else {
-                table.rollback_to(snap);
                 }
                 if let Some(from_module) = visible_from_module {
                     if !db.function_visibility(m).is_visible_from(db.upcast(), from_module) {
@@ -1028,12 +1024,38 @@ fn is_valid_candidate(
                 }
 
                 true
+            })
         }
         AssocItemId::ConstId(c) => {
             let data = db.const_data(c);
-            // TODO check unify self ty
+            if receiver_ty.is_some() {
-            // TODO check visibility
+                return false;
-            name.map_or(true, |name| data.name.as_ref() == Some(name)) && receiver_ty.is_none()
+            }
+            if let Some(name) = name {
+                if data.name.as_ref() != Some(name) {
+                    return false;
+                }
+            }
+            if let Some(from_module) = visible_from_module {
+                if !db.const_visibility(c).is_visible_from(db.upcast(), from_module) {
+                    cov_mark::hit!(const_candidate_not_visible);
+                    return false;
+                }
+            }
+            if let ItemContainerId::ImplId(impl_id) = c.lookup(db.upcast()).container {
+                let self_ty_matches = table.run_in_snapshot(|table| {
+                    let subst =
+                        TyBuilder::subst_for_def(db, c).fill_with_inference_vars(table).build();
+                    let expected_self_ty =
+                        subst.apply(db.impl_self_ty(impl_id).skip_binders().clone(), Interner);
+                    table.unify(&expected_self_ty, &self_ty)
+                });
+                if !self_ty_matches {
+                    cov_mark::hit!(const_candidate_self_type_mismatch);
+                    return false;
+                }
+            }
+            true
         }
         _ => false,
     }

crates/hir_ty/src/tests/method_resolution.rs

@@ -953,6 +953,33 @@ fn main() {
     );
 }
 
+#[test]
+fn method_resolution_overloaded_const() {
+    cov_mark::check!(const_candidate_self_type_mismatch);
+    check_types(
+        r#"
+struct Wrapper<T>(T);
+struct Foo<T>(T);
+struct Bar<T>(T);
+
+impl<T> Wrapper<Foo<T>> {
+    pub const VALUE: Foo<T>;
+}
+
+impl<T> Wrapper<Bar<T>> {
+    pub const VALUE: Bar<T>;
+}
+
+fn main() {
+    let a = Wrapper::<Foo<f32>>::VALUE;
+    let b = Wrapper::<Bar<f32>>::VALUE;
+    (a, b);
+  //^^^^^^ (Foo<f32>, Bar<f32>)
+}
+"#,
+    );
+}
+
 #[test]
 fn method_resolution_encountering_fn_type() {
     check_types(
@@ -1256,6 +1283,37 @@ mod b {
     )
 }
 
+#[test]
+fn trait_vs_private_inherent_const() {
+    cov_mark::check!(const_candidate_not_visible);
+    check(
+        r#"
+mod a {
+    pub struct Foo;
+    impl Foo {
+        const VALUE: u32 = 2;
+    }
+    pub trait Trait {
+        const VALUE: usize;
+    }
+    impl Trait for Foo {
+        const VALUE: usize = 3;
+    }
+
+    fn foo() {
+        let x = Foo::VALUE;
+            //  ^^^^^^^^^^ type: u32
+    }
+}
+use a::Trait;
+fn foo() {
+    let x = a::Foo::VALUE;
+         // ^^^^^^^^^^^^^ type: usize
+}
+"#,
+    )
+}
+
 #[test]
 fn trait_impl_in_unnamed_const() {
     check_types(