Repository: rust-lang/rust-analyzer
Pull request #12554 (2022-06-16 10:00:38)

fix: local items should not be completed in parent signature
fixes #11959 

> We get a Bar completion for the following snippet which is wrong as the item is not visible in that position.
> ``` rust
> fn foo() -> $0 {
>    struct Bar;
> }
> ```

I investigated the problem and found that the scope of the cursor offset, also `CompletionContext.scope` is the body of the function



Patch:
diff --git a/crates/hir-def/src/data.rs b/crates/hir-def/src/data.rs
index 7436f0100b..f2d8318f7d 100644
--- a/crates/hir-def/src/data.rs
+++ b/crates/hir-def/src/data.rs
@@ -40,11 +40,6 @@ impl FunctionData {
         let cfg_options = &crate_graph[krate].cfg_options;
         let item_tree = loc.id.item_tree(db);
         let func = &item_tree[loc.id.value];
-        let visibility = if let ItemContainerId::TraitId(trait_id) = loc.container {
-            db.trait_data(trait_id).visibility.clone()
-        } else {
-            item_tree[func.visibility].clone()
-        };
 
         let enabled_params = func
             .params
@@ -98,7 +93,7 @@ impl FunctionData {
             ret_type: func.ret_type.clone(),
             async_ret_type: func.async_ret_type.clone(),
             attrs: item_tree.attrs(db, krate, ModItem::from(loc.id.value).into()),
-            visibility,
+            visibility: item_tree[func.visibility].clone(),
             abi: func.abi.clone(),
             legacy_const_generics_indices,
             flags,
@@ -176,16 +171,11 @@ impl TypeAliasData {
         let loc = typ.lookup(db);
         let item_tree = loc.id.item_tree(db);
         let typ = &item_tree[loc.id.value];
-        let visibility = if let ItemContainerId::TraitId(trait_id) = loc.container {
-            db.trait_data(trait_id).visibility.clone()
-        } else {
-            item_tree[typ.visibility].clone()
-        };
 
         Arc::new(TypeAliasData {
             name: typ.name.clone(),
             type_ref: typ.type_ref.clone(),
-            visibility,
+            visibility: item_tree[typ.visibility].clone(),
             is_extern: matches!(loc.container, ItemContainerId::ExternBlockId(_)),
             bounds: typ.bounds.to_vec(),
         })
@@ -395,16 +385,11 @@ impl ConstData {
         let loc = konst.lookup(db);
         let item_tree = loc.id.item_tree(db);
         let konst = &item_tree[loc.id.value];
-        let visibility = if let ItemContainerId::TraitId(trait_id) = loc.container {
-            db.trait_data(trait_id).visibility.clone()
-        } else {
-            item_tree[konst.visibility].clone()
-        };
 
         Arc::new(ConstData {
             name: konst.name.clone(),
             type_ref: konst.type_ref.clone(),
-            visibility,
+            visibility: item_tree[konst.visibility].clone(),
         })
     }
 }
diff --git a/crates/hir-def/src/item_tree/lower.rs b/crates/hir-def/src/item_tree/lower.rs
index 7f2551e941..cdae0d0803 100644
--- a/crates/hir-def/src/item_tree/lower.rs
+++ b/crates/hir-def/src/item_tree/lower.rs
@@ -1,6 +1,6 @@
 //! AST -> `ItemTree` lowering code.
 
-use std::{collections::hash_map::Entry, sync::Arc};
+use std::{collections::hash_map::Entry, mem, sync::Arc};
 
 use hir_expand::{ast_id_map::AstIdMap, hygiene::Hygiene, HirFileId};
 use syntax::ast::{self, HasModuleItem};
@@ -21,6 +21,7 @@ pub(super) struct Ctx<'a> {
     tree: ItemTree,
     source_ast_id_map: Arc<AstIdMap>,
     body_ctx: crate::body::LowerCtx<'a>,
+    forced_visibility: Option<RawVisibilityId>,
 }
 
 impl<'a> Ctx<'a> {
@@ -30,6 +31,7 @@ impl<'a> Ctx<'a> {
             tree: ItemTree::default(),
             source_ast_id_map: db.ast_id_map(file),
             body_ctx: crate::body::LowerCtx::new(db, file),
+            forced_visibility: None,
         }
     }
 
@@ -223,10 +225,11 @@ impl<'a> Ctx<'a> {
         let visibility = self.lower_visibility(enum_);
         let name = enum_.name()?.as_name();
         let generic_params = self.lower_generic_params(GenericsOwner::Enum, enum_);
-        let variants = match &enum_.variant_list() {
-            Some(variant_list) => self.lower_variants(variant_list),
-            None => IdxRange::new(self.next_variant_idx()..self.next_variant_idx()),
-        };
+        let variants =
+            self.with_inherited_visibility(visibility, |this| match &enum_.variant_list() {
+                Some(variant_list) => this.lower_variants(variant_list),
+                None => IdxRange::new(this.next_variant_idx()..this.next_variant_idx()),
+            });
         let ast_id = self.source_ast_id_map.ast_id(enum_);
         let res = Enum { name, visibility, generic_params, variants, ast_id };
         Some(id(self.data().enums.alloc(res)))
@@ -437,15 +440,18 @@ impl<'a> Ctx<'a> {
         let is_auto = trait_def.auto_token().is_some();
         let is_unsafe = trait_def.unsafe_token().is_some();
         let items = trait_def.assoc_item_list().map(|list| {
-            list.assoc_items()
-                .filter_map(|item| {
-                    let attrs = RawAttrs::new(self.db, &item, self.hygiene());
-                    self.lower_assoc_item(&item).map(|item| {
-                        self.add_attrs(ModItem::from(item).into(), attrs);
-                        item
+            let db = self.db;
+            self.with_inherited_visibility(visibility, |this| {
+                list.assoc_items()
+                    .filter_map(|item| {
+                        let attrs = RawAttrs::new(db, &item, this.hygiene());
+                        this.lower_assoc_item(&item).map(|item| {
+                            this.add_attrs(ModItem::from(item).into(), attrs);
+                            item
+                        })
                     })
-                })
-                .collect()
+                    .collect()
+            })
         });
         let ast_id = self.source_ast_id_map.ast_id(trait_def);
         let res = Trait {
@@ -616,7 +622,13 @@ impl<'a> Ctx<'a> {
     }
 
     fn lower_visibility(&mut self, item: &dyn ast::HasVisibility) -> RawVisibilityId {
-        let vis = RawVisibility::from_ast_with_hygiene(self.db, item.visibility(), self.hygiene());
+        let vis = match self.forced_visibility {
+            Some(vis) => return vis,
+            None => {
+                RawVisibility::from_ast_with_hygiene(self.db, item.visibility(), self.hygiene())
+            }
+        };
+
         self.data().vis.alloc(vis)
     }
 
@@ -637,6 +649,18 @@ impl<'a> Ctx<'a> {
         }
     }
 
+    /// Forces the visibility `vis` to be used for all items lowered during execution of `f`.
+    fn with_inherited_visibility<R>(
+        &mut self,
+        vis: RawVisibilityId,
+        f: impl FnOnce(&mut Self) -> R,
+    ) -> R {
+        let old = mem::replace(&mut self.forced_visibility, Some(vis));
+        let res = f(self);
+        self.forced_visibility = old;
+        res
+    }
+
     fn next_field_idx(&self) -> Idx<Field> {
         Idx::from_raw(RawIdx::from(
             self.tree.data.as_ref().map_or(0, |data| data.fields.len() as u32),
diff --git a/crates/hir/src/source_analyzer.rs b/crates/hir/src/source_analyzer.rs
index b5e6d99093..d624d37577 100644
--- a/crates/hir/src/source_analyzer.rs
+++ b/crates/hir/src/source_analyzer.rs
@@ -67,7 +67,10 @@ impl SourceAnalyzer {
         let scopes = db.expr_scopes(def);
         let scope = match offset {
             None => scope_for(&scopes, &source_map, node),
-            Some(offset) => scope_for_offset(db, &scopes, &source_map, node.with_value(offset)),
+            Some(offset) => {
+                let file_id = node.file_id.original_file(db.upcast());
+                scope_for_offset(db, &scopes, &source_map, InFile::new(file_id.into(), offset))
+            }
         };
         let resolver = resolver_for_scope(db.upcast(), def, scope);
         SourceAnalyzer {
@@ -88,7 +91,10 @@ impl SourceAnalyzer {
         let scopes = db.expr_scopes(def);
         let scope = match offset {
             None => scope_for(&scopes, &source_map, node),
-            Some(offset) => scope_for_offset(db, &scopes, &source_map, node.with_value(offset)),
+            Some(offset) => {
+                let file_id = node.file_id.original_file(db.upcast());
+                scope_for_offset(db, &scopes, &source_map, InFile::new(file_id.into(), offset))
+            }
         };
         let resolver = resolver_for_scope(db.upcast(), def, scope);
         SourceAnalyzer { resolver, def: Some((def, body, source_map)), infer: None, file_id }
@@ -600,13 +606,11 @@ fn scope_for_offset(
             .filter(|it| it.value.kind() == SyntaxKind::MACRO_CALL)?;
             Some((source.value.text_range(), scope))
         })
-        // find containing scope
-        .min_by_key(|(expr_range, _scope)| {
-            (
-                !(expr_range.start() <= offset.value && offset.value <= expr_range.end()),
-                expr_range.len(),
-            )
+        .filter(|(expr_range, _scope)| {
+            expr_range.start() <= offset.value && offset.value <= expr_range.end()
         })
+        // find containing scope
+        .min_by_key(|(expr_range, _scope)| expr_range.len())
         .map(|(expr_range, scope)| {
             adjust(db, scopes, source_map, expr_range, offset).unwrap_or(*scope)
         })
diff --git a/crates/ide-completion/src/completions/dot.rs b/crates/ide-completion/src/completions/dot.rs
index 4eb1fccd7d..a11652ca30 100644
--- a/crates/ide-completion/src/completions/dot.rs
+++ b/crates/ide-completion/src/completions/dot.rs
@@ -117,20 +117,13 @@ fn complete_methods(
 mod tests {
     use expect_test::{expect, Expect};
 
-    use crate::tests::{
-        check_edit, completion_list_no_kw, completion_list_no_kw_with_private_editable,
-    };
+    use crate::tests::{check_edit, completion_list_no_kw};
 
     fn check(ra_fixture: &str, expect: Expect) {
         let actual = completion_list_no_kw(ra_fixture);
         expect.assert_eq(&actual);
     }
 
-    fn check_with_private_editable(ra_fixture: &str, expect: Expect) {
-        let actual = completion_list_no_kw_with_private_editable(ra_fixture);
-        expect.assert_eq(&actual);
-    }
-
     #[test]
     fn test_struct_field_and_method_completion() {
         check(
@@ -207,101 +200,6 @@ pub mod m {
 }
 //- /main.rs crate:main deps:lib new_source_root:local
 fn foo(a: lib::m::A) { a.$0 }
-"#,
-            expect![[r#"
-                fd pub_field u32
-            "#]],
-        );
-
-        check(
-            r#"
-//- /lib.rs crate:lib new_source_root:library
-pub mod m {
-    pub struct A {
-        private_field: u32,
-        pub pub_field: u32,
-        pub(crate) crate_field: u32,
-        pub(super) super_field: u32,
-    }
-}
-//- /main.rs crate:main deps:lib new_source_root:local
-fn foo(a: lib::m::A) { a.$0 }
-"#,
-            expect![[r#"
-                fd pub_field u32
-            "#]],
-        );
-
-        check(
-            r#"
-//- /lib.rs crate:lib new_source_root:library
-pub mod m {
-    pub struct A(
-        i32,
-        pub f64,
-    );
-}
-//- /main.rs crate:main deps:lib new_source_root:local
-fn foo(a: lib::m::A) { a.$0 }
-"#,
-            expect![[r#"
-                fd 1 f64
-            "#]],
-        );
-
-        check(
-            r#"
-//- /lib.rs crate:lib new_source_root:local
-pub struct A {}
-mod m {
-    impl super::A {
-        fn private_method(&self) {}
-        pub(crate) fn crate_method(&self) {}
-        pub fn pub_method(&self) {}
-    }
-}
-//- /main.rs crate:main deps:lib new_source_root:local
-fn foo(a: lib::A) { a.$0 }
-"#,
-            expect![[r#"
-                me pub_method() fn(&self)
-            "#]],
-        );
-        check(
-            r#"
-//- /lib.rs crate:lib new_source_root:library
-pub struct A {}
-mod m {
-    impl super::A {
-        fn private_method(&self) {}
-        pub(crate) fn crate_method(&self) {}
-        pub fn pub_method(&self) {}
-    }
-}
-//- /main.rs crate:main deps:lib new_source_root:local
-fn foo(a: lib::A) { a.$0 }
-"#,
-            expect![[r#"
-                me pub_method() fn(&self)
-            "#]],
-        );
-    }
-
-    #[test]
-    fn test_visibility_filtering_with_private_editable_enabled() {
-        check_with_private_editable(
-            r#"
-//- /lib.rs crate:lib new_source_root:local
-pub mod m {
-    pub struct A {
-        private_field: u32,
-        pub pub_field: u32,
-        pub(crate) crate_field: u32,
-        pub(super) super_field: u32,
-    }
-}
-//- /main.rs crate:main deps:lib new_source_root:local
-fn foo(a: lib::m::A) { a.$0 }
 "#,
             expect![[r#"
                 fd crate_field   u32
@@ -311,7 +209,7 @@ fn foo(a: lib::m::A) { a.$0 }
             "#]],
         );
 
-        check_with_private_editable(
+        check(
             r#"
 //- /lib.rs crate:lib new_source_root:library
 pub mod m {
@@ -330,7 +228,7 @@ fn foo(a: lib::m::A) { a.$0 }
             "#]],
         );
 
-        check_with_private_editable(
+        check(
             r#"
 //- /lib.rs crate:lib new_source_root:library
 pub mod m {
@@ -347,7 +245,7 @@ fn foo(a: lib::m::A) { a.$0 }
             "#]],
         );
 
-        check_with_private_editable(
+        check(
             r#"
 //- /lib.rs crate:lib new_source_root:local
 pub struct A {}
@@ -367,7 +265,7 @@ fn foo(a: lib::A) { a.$0 }
                 me pub_method()     fn(&self)
             "#]],
         );
-        check_with_private_editable(
+        check(
             r#"
 //- /lib.rs crate:lib new_source_root:library
 pub struct A {}


Test patch:
diff --git a/crates/hir-def/src/item_tree/tests.rs b/crates/hir-def/src/item_tree/tests.rs
index 5cdf36cc61..fb3811dbd5 100644
--- a/crates/hir-def/src/item_tree/tests.rs
+++ b/crates/hir-def/src/item_tree/tests.rs
@@ -358,3 +358,40 @@ trait Tr<'a, T: 'a>: Super where Self: for<'a> Tr<'a, T> {}
         "#]],
     )
 }
+
+#[test]
+fn inherit_visibility() {
+    check(
+        r#"
+pub(crate) enum En {
+    Var1(u8),
+    Var2 {
+        fld: u8,
+    },
+}
+
+pub(crate) trait Tr {
+    fn f();
+    fn method(&self) {}
+}
+        "#,
+        expect![[r#"
+            pub(crate) enum En {
+                Var1(
+                    pub(crate) 0: u8,
+                ),
+                Var2 {
+                    pub(crate) fld: u8,
+                },
+            }
+
+            pub(crate) trait Tr<Self> {
+                pub(crate) fn f() -> ();
+
+                pub(crate) fn method(
+                    _: &Self,  // self
+                ) -> () { ... }
+            }
+        "#]],
+    )
+}
diff --git a/crates/ide-completion/src/tests.rs b/crates/ide-completion/src/tests.rs
index 4be6acbe84..d30ff77bab 100644
--- a/crates/ide-completion/src/tests.rs
+++ b/crates/ide-completion/src/tests.rs
@@ -65,7 +65,7 @@ pub(crate) const TEST_CONFIG: CompletionConfig = CompletionConfig {
     enable_postfix_completions: true,
     enable_imports_on_the_fly: true,
     enable_self_on_the_fly: true,
-    enable_private_editable: false,
+    enable_private_editable: true,
     callable: Some(CallableSnippets::FillArguments),
     snippet_cap: SnippetCap::new(true),
     insert_use: InsertUseConfig {
@@ -86,12 +86,6 @@ pub(crate) fn completion_list_no_kw(ra_fixture: &str) -> String {
     completion_list_with_config(TEST_CONFIG, ra_fixture, false, None)
 }
 
-pub(crate) fn completion_list_no_kw_with_private_editable(ra_fixture: &str) -> String {
-    let mut config = TEST_CONFIG.clone();
-    config.enable_private_editable = true;
-    completion_list_with_config(config, ra_fixture, false, None)
-}
-
 pub(crate) fn completion_list_with_trigger_character(
     ra_fixture: &str,
     trigger_character: Option<char>,
diff --git a/crates/ide-completion/src/tests/special.rs b/crates/ide-completion/src/tests/special.rs
index 4535923b28..6195537a18 100644
--- a/crates/ide-completion/src/tests/special.rs
+++ b/crates/ide-completion/src/tests/special.rs
@@ -636,109 +636,3 @@ fn bar() -> Bar {
             "#]],
     )
 }
-
-#[test]
-fn completes_fn_in_pub_trait_generated_by_macro() {
-    check(
-        r#"
-mod other_mod {
-    macro_rules! make_method {
-        ($name:ident) => {
-            fn $name(&self) {}
-        };
-    }
-
-    pub trait MyTrait {
-        make_method! { by_macro }
-        fn not_by_macro(&self) {}
-    }
-
-    pub struct Foo {}
-
-    impl MyTrait for Foo {}
-}
-
-fn main() {
-    use other_mod::{Foo, MyTrait};
-    let f = Foo {};
-    f.$0
-}
-"#,
-        expect![[r#"
-            me by_macro() (as MyTrait) fn(&self)
-            me not_by_macro() (as MyTrait) fn(&self)
-        "#]],
-    )
-}
-
-#[test]
-fn completes_fn_in_pub_trait_generated_by_recursive_macro() {
-    check(
-        r#"
-mod other_mod {
-    macro_rules! make_method {
-        ($name:ident) => {
-            fn $name(&self) {}
-        };
-    }
-
-    macro_rules! make_trait {
-        () => {
-            pub trait MyTrait {
-                make_method! { by_macro }
-                fn not_by_macro(&self) {}
-            }
-        }
-    }
-
-    make_trait!();
-
-    pub struct Foo {}
-
-    impl MyTrait for Foo {}
-}
-
-fn main() {
-    use other_mod::{Foo, MyTrait};
-    let f = Foo {};
-    f.$0
-}
-"#,
-        expect![[r#"
-            me by_macro() (as MyTrait) fn(&self)
-            me not_by_macro() (as MyTrait) fn(&self)
-        "#]],
-    )
-}
-
-#[test]
-fn completes_const_in_pub_trait_generated_by_macro() {
-    check(
-        r#"
-mod other_mod {
-    macro_rules! make_const {
-        ($name:ident) => {
-            const $name: u8 = 1;
-        };
-    }
-
-    pub trait MyTrait {
-        make_const! { by_macro }
-    }
-
-    pub struct Foo {}
-
-    impl MyTrait for Foo {}
-}
-
-fn main() {
-    use other_mod::{Foo, MyTrait};
-    let f = Foo {};
-    Foo::$0
-}
-"#,
-        expect![[r#"
-            ct by_macro (as MyTrait) pub const by_macro: u8
-        "#]],
-    )
-}
diff --git a/crates/ide-completion/src/tests/type_pos.rs b/crates/ide-completion/src/tests/type_pos.rs
index 1e5e86eef5..8943d303b6 100644
--- a/crates/ide-completion/src/tests/type_pos.rs
+++ b/crates/ide-completion/src/tests/type_pos.rs
@@ -89,6 +89,37 @@ fn x<'lt, T, const C: usize>() -> $0
     );
 }
 
+#[test]
+fn fn_return_type_no_local_items() {
+    check(
+        r#"
+fn foo() -> B$0 {
+    struct Bar;
+    enum Baz {}
+    union Bax {
+        i: i32,
+        f: f32
+    }
+}
+"#,
+        expect![[r#"
+        en Enum
+        ma makro!(…) macro_rules! makro
+        md module
+        st Record
+        st Tuple
+        st Unit
+        tt Trait
+        un Union
+        bt u32
+        it ()
+        kw crate::
+        kw self::
+        kw super::
+    "#]],
+    )
+}
+
 #[test]
 fn inferred_type_const() {
     check(