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

fix: add missing visibility for auto-import names #6205

Merged
merged 3 commits into from
Oct 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion compiler/noirc_frontend/src/elaborator/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1385,7 +1385,7 @@ impl<'context> Elaborator<'context> {
}

if let Some(name) = name {
self.interner.register_global(global_id, name, self.module_id());
self.interner.register_global(global_id, name, global.visibility, self.module_id());
}

self.local_module = old_module;
Expand Down
1 change: 1 addition & 0 deletions compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ pub struct UnresolvedGlobal {
pub module_id: LocalModuleId,
pub global_id: GlobalId,
pub stmt_def: LetStatement,
pub visibility: ItemVisibility,
}

pub struct ModuleAttribute {
Expand Down
18 changes: 14 additions & 4 deletions compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,12 @@
if context.def_interner.is_in_lsp_mode() {
let parent_module_id = ModuleId { krate, local_id: self.module_id };
let name = name.to_string();
context.def_interner.register_type_alias(type_alias_id, name, parent_module_id);
context.def_interner.register_type_alias(
type_alias_id,
name,
visibility,
parent_module_id,
);
}
}
errors
Expand Down Expand Up @@ -589,7 +594,12 @@

if context.def_interner.is_in_lsp_mode() {
let parent_module_id = ModuleId { krate, local_id: self.module_id };
context.def_interner.register_trait(trait_id, name.to_string(), parent_module_id);
context.def_interner.register_trait(
trait_id,
name.to_string(),
visibility,
parent_module_id,
);
}

self.def_collector.items.traits.insert(trait_id, unresolved);
Expand Down Expand Up @@ -823,7 +833,7 @@
// if it's an inline module, or the first char of a the file if it's an external module.
// - `location` will always point to the token "foo" in `mod foo` regardless of whether
// it's inline or external.
// Eventually the location put in `ModuleData` is used for codelenses about `contract`s,

Check warning on line 836 in compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (codelenses)
// so we keep using `location` so that it continues to work as usual.
let location = Location::new(mod_name.span(), mod_location.file);
let new_module =
Expand Down Expand Up @@ -866,7 +876,7 @@
);

if interner.is_in_lsp_mode() {
interner.register_module(mod_id, mod_name.0.contents.clone());
interner.register_module(mod_id, visibility, mod_name.0.contents.clone());
}
}

Expand Down Expand Up @@ -1207,7 +1217,7 @@

interner.set_doc_comments(ReferenceId::Global(global_id), doc_comments);

let global = UnresolvedGlobal { file_id, module_id, global_id, stmt_def: global };
let global = UnresolvedGlobal { file_id, module_id, global_id, stmt_def: global, visibility };
(global, error)
}

Expand Down
24 changes: 15 additions & 9 deletions compiler/noirc_frontend/src/locations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -278,20 +278,23 @@ impl NodeInterner {
.next()
}

pub(crate) fn register_module(&mut self, id: ModuleId, name: String) {
let visibility = ItemVisibility::Public;
pub(crate) fn register_module(
&mut self,
id: ModuleId,
visibility: ItemVisibility,
name: String,
) {
self.register_name_for_auto_import(name, ModuleDefId::ModuleId(id), visibility, None);
}

pub(crate) fn register_global(
&mut self,
id: GlobalId,
name: String,
visibility: ItemVisibility,
parent_module_id: ModuleId,
) {
self.add_definition_location(ReferenceId::Global(id), Some(parent_module_id));

let visibility = ItemVisibility::Public;
self.register_name_for_auto_import(name, ModuleDefId::GlobalId(id), visibility, None);
}

Expand All @@ -306,22 +309,25 @@ impl NodeInterner {
self.register_name_for_auto_import(name, ModuleDefId::TypeId(id), visibility, None);
}

pub(crate) fn register_trait(&mut self, id: TraitId, name: String, parent_module_id: ModuleId) {
pub(crate) fn register_trait(
&mut self,
id: TraitId,
name: String,
visibility: ItemVisibility,
parent_module_id: ModuleId,
) {
self.add_definition_location(ReferenceId::Trait(id), Some(parent_module_id));

let visibility = ItemVisibility::Public;
self.register_name_for_auto_import(name, ModuleDefId::TraitId(id), visibility, None);
}

pub(crate) fn register_type_alias(
&mut self,
id: TypeAliasId,
name: String,
visibility: ItemVisibility,
parent_module_id: ModuleId,
) {
self.add_definition_location(ReferenceId::Alias(id), Some(parent_module_id));

let visibility = ItemVisibility::Public;
self.register_name_for_auto_import(name, ModuleDefId::TypeAliasId(id), visibility, None);
}

Expand Down
8 changes: 4 additions & 4 deletions tooling/lsp/src/requests/code_action/import_or_qualify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ fn foo(x: SomeTypeInBar) {}"#;
let src = r#"
mod foo {
mod bar {
mod some_module_in_bar {}
pub mod some_module_in_bar {}
}
}

Expand All @@ -198,7 +198,7 @@ fn foo(x: SomeTypeInBar) {}"#;
let expected = r#"
mod foo {
mod bar {
mod some_module_in_bar {}
pub mod some_module_in_bar {}
}
}

Expand All @@ -216,7 +216,7 @@ fn foo(x: SomeTypeInBar) {}"#;

let src = r#"mod foo {
mod bar {
mod some_module_in_bar {}
pub(crate) mod some_module_in_bar {}
}
}

Expand All @@ -228,7 +228,7 @@ fn main() {

mod foo {
mod bar {
mod some_module_in_bar {}
pub(crate) mod some_module_in_bar {}
}
}

Expand Down
54 changes: 53 additions & 1 deletion tooling/lsp/src/requests/completion/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,14 +133,14 @@
#[test]
async fn test_use_first_segment() {
let src = r#"
mod foobaz {}

Check warning on line 136 in tooling/lsp/src/requests/completion/tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (foobaz)
mod foobar {}
use foob>|<

Check warning on line 138 in tooling/lsp/src/requests/completion/tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (foob)
"#;

assert_completion(
src,
vec![module_completion_item("foobaz"), module_completion_item("foobar")],

Check warning on line 143 in tooling/lsp/src/requests/completion/tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (foobaz)
)
.await;
}
Expand Down Expand Up @@ -303,7 +303,7 @@
mod bar {
mod something {}

use super::foob>|<

Check warning on line 306 in tooling/lsp/src/requests/completion/tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (foob)
}
"#;

Expand Down Expand Up @@ -1575,7 +1575,7 @@
async fn test_auto_import_suggests_modules_too() {
let src = r#"
mod foo {
mod barbaz {
pub mod barbaz {

Check warning on line 1578 in tooling/lsp/src/requests/completion/tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (barbaz)
fn hello_world() {}
}
}
Expand All @@ -1588,11 +1588,11 @@
assert_eq!(items.len(), 1);

let item = &items[0];
assert_eq!(item.label, "barbaz");

Check warning on line 1591 in tooling/lsp/src/requests/completion/tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (barbaz)
assert_eq!(
item.label_details,
Some(CompletionItemLabelDetails {
detail: Some("(use foo::barbaz)".to_string()),

Check warning on line 1595 in tooling/lsp/src/requests/completion/tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (barbaz)
description: None
})
);
Expand Down Expand Up @@ -1699,7 +1699,7 @@
async fn test_completes_after_first_letter_of_path() {
let src = r#"
fn main() {
h>|<ello();

Check warning on line 1702 in tooling/lsp/src/requests/completion/tests.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (ello)
}

fn hello_world() {}
Expand Down Expand Up @@ -2271,4 +2271,56 @@
)
.await;
}

#[test]
async fn test_does_not_auto_import_private_global() {
let src = r#"mod moo {
global foobar = 1;
}

fn main() {
fooba>|<
}"#;

assert_completion(src, Vec::new()).await;
}

#[test]
async fn test_does_not_auto_import_private_type_alias() {
let src = r#"mod moo {
type foobar = i32;
}

fn main() {
fooba>|<
}"#;

assert_completion(src, Vec::new()).await;
}

#[test]
async fn test_does_not_auto_import_private_trait() {
let src = r#"mod moo {
trait Foobar {}
}

fn main() {
Fooba>|<
}"#;

assert_completion(src, Vec::new()).await;
}

#[test]
async fn test_does_not_auto_import_private_module() {
let src = r#"mod moo {
mod foobar {}
}

fn main() {
fooba>|<
}"#;

assert_completion(src, Vec::new()).await;
}
}
Loading