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: Do not warn on unused functions marked with #[export] #6625

Merged
merged 3 commits into from
Nov 26, 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
4 changes: 3 additions & 1 deletion compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -862,7 +862,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 865 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 = ModuleData::new(
Expand Down Expand Up @@ -944,6 +944,7 @@
} else {
function.name() == MAIN_FUNCTION
};
let has_export = function.def.attributes.has_export();

let name = function.name_ident().clone();
let func_id = interner.push_empty_fn();
Expand All @@ -954,7 +955,7 @@
interner.register_function(func_id, &function.def);
}

if !is_test && !is_entry_point_function {
if !is_test && !is_entry_point_function && !has_export {
let item = UnusedItem::Function(func_id);
usage_tracker.add_unused_item(module, name.clone(), item, visibility);
}
Expand Down Expand Up @@ -1257,6 +1258,7 @@
// Add the statement to the scope so its path can be looked up later
let result = def_map.modules[module_id.0].declare_global(name.clone(), visibility, global_id);

// Globals marked as ABI don't have to be used.
if !is_abi {
let parent_module_id = ModuleId { krate: crate_id, local_id: module_id };
usage_tracker.add_unused_item(
Expand Down
6 changes: 1 addition & 5 deletions compiler/noirc_frontend/src/hir/def_map/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -193,11 +193,7 @@ impl CrateDefMap {
module.value_definitions().filter_map(|id| {
if let Some(func_id) = id.as_function() {
let attributes = interner.function_attributes(&func_id);
if attributes.secondary.contains(&SecondaryAttribute::Export) {
Some(func_id)
} else {
None
}
attributes.has_export().then_some(func_id)
} else {
None
}
Expand Down
18 changes: 13 additions & 5 deletions compiler/noirc_frontend/src/lexer/token.rs
Original file line number Diff line number Diff line change
Expand Up @@ -676,9 +676,7 @@ impl Attributes {
/// This is useful for finding out if we should compile a contract method
/// as an entry point or not.
pub fn has_contract_library_method(&self) -> bool {
self.secondary
.iter()
.any(|attribute| attribute == &SecondaryAttribute::ContractLibraryMethod)
self.has_secondary_attr(&SecondaryAttribute::ContractLibraryMethod)
}

pub fn is_test_function(&self) -> bool {
Expand Down Expand Up @@ -718,11 +716,21 @@ impl Attributes {
}

pub fn has_varargs(&self) -> bool {
self.secondary.iter().any(|attr| matches!(attr, SecondaryAttribute::Varargs))
self.has_secondary_attr(&SecondaryAttribute::Varargs)
}

pub fn has_use_callers_scope(&self) -> bool {
self.secondary.iter().any(|attr| matches!(attr, SecondaryAttribute::UseCallersScope))
self.has_secondary_attr(&SecondaryAttribute::UseCallersScope)
}

/// True if the function is marked with an `#[export]` attribute.
pub fn has_export(&self) -> bool {
self.has_secondary_attr(&SecondaryAttribute::Export)
}

/// Check if secondary attributes contain a specific instance.
pub fn has_secondary_attr(&self, attr: &SecondaryAttribute) -> bool {
self.secondary.contains(attr)
}
}

Expand Down
38 changes: 30 additions & 8 deletions compiler/noirc_frontend/src/tests/unused_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -224,9 +224,31 @@ fn does_not_warn_on_unused_global_if_it_has_an_abi_attribute() {
assert_no_errors(src);
}

#[test]
fn does_not_warn_on_unused_struct_if_it_has_an_abi_attribute() {
let src = r#"
#[abi(dummy)]
struct Foo { bar: u8 }

fn main() {}
"#;
assert_no_errors(src);
}

#[test]
fn does_not_warn_on_unused_function_if_it_has_an_export_attribute() {
let src = r#"
#[export]
fn foo() {}

fn main() {}
"#;
assert_no_errors(src);
}

#[test]
fn no_warning_on_inner_struct_when_parent_is_used() {
let src = r#"
let src = r#"
struct Bar {
inner: [Field; 3],
}
Expand All @@ -247,7 +269,7 @@ fn no_warning_on_inner_struct_when_parent_is_used() {

#[test]
fn no_warning_on_struct_if_it_has_an_abi_attribute() {
let src = r#"
let src = r#"
#[abi(functions)]
struct Foo {
a: Field,
Expand All @@ -260,7 +282,7 @@ fn no_warning_on_struct_if_it_has_an_abi_attribute() {

#[test]
fn no_warning_on_indirect_struct_if_it_has_an_abi_attribute() {
let src = r#"
let src = r#"
struct Bar {
field: Field,
}
Expand All @@ -277,7 +299,7 @@ fn no_warning_on_indirect_struct_if_it_has_an_abi_attribute() {

#[test]
fn no_warning_on_self_in_trait_impl() {
let src = r#"
let src = r#"
struct Bar {}

trait Foo {
Expand All @@ -298,18 +320,18 @@ fn no_warning_on_self_in_trait_impl() {
#[test]
fn resolves_trait_where_clause_in_the_correct_module() {
// This is a regression test for https://github.com/noir-lang/noir/issues/6479
let src = r#"
let src = r#"
mod foo {
pub trait Foo {}
}

use foo::Foo;

pub trait Bar<T>
where
T: Foo,
{}

fn main() {}
"#;
assert_no_errors(src);
Expand Down
3 changes: 3 additions & 0 deletions compiler/noirc_frontend/src/usage_tracker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ pub struct UsageTracker {
}

impl UsageTracker {
/// Register an item as unused, waiting to be marked as used later.
/// Things that should not emit warnings should not be added at all.
pub(crate) fn add_unused_item(
&mut self,
module_id: ModuleId,
Expand Down Expand Up @@ -73,6 +75,7 @@ impl UsageTracker {
};
}

/// Get all the unused items per module.
pub fn unused_items(&self) -> &HashMap<ModuleId, HashMap<Ident, UnusedItem>> {
&self.unused_items
}
Expand Down
Loading