Skip to content

Commit

Permalink
feat: visibility for traits (#6056)
Browse files Browse the repository at this point in the history
# Description

## Problem

Part of #4515
Fixes #4775

## Summary

Traits can now be marked as `pub` or `pub(crate)`, and a warning will
happen if a trait is unused.

## Additional Context

## Documentation

Check one:
- [ ] No documentation needed.
- [x] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
  • Loading branch information
asterite authored Sep 17, 2024
1 parent 199be58 commit 5bbd9ba
Show file tree
Hide file tree
Showing 16 changed files with 107 additions and 44 deletions.
1 change: 1 addition & 0 deletions compiler/noirc_frontend/src/ast/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ pub struct NoirTrait {
pub span: Span,
pub items: Vec<Documented<TraitItem>>,
pub attributes: Vec<SecondaryAttribute>,
pub visibility: ItemVisibility,
}

/// Any declaration inside the body of a trait that a user is required to
Expand Down
16 changes: 14 additions & 2 deletions compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -392,8 +392,20 @@ impl<'a> ModCollector<'a> {
context.def_interner.set_doc_comments(ReferenceId::Trait(trait_id), doc_comments);

// Add the trait to scope so its path can be looked up later
let result = self.def_collector.def_map.modules[self.module_id.0]
.declare_trait(name.clone(), trait_id);
let visibility = trait_definition.visibility;
let result = self.def_collector.def_map.modules[self.module_id.0].declare_trait(
name.clone(),
visibility,
trait_id,
);

let parent_module_id = ModuleId { krate, local_id: self.module_id };
context.def_interner.usage_tracker.add_unused_item(
parent_module_id,
name.clone(),
UnusedItem::Trait(trait_id),
visibility,
);

if let Err((first_def, second_def)) = result {
let error = DefCollectorErrorKind::Duplicate {
Expand Down
9 changes: 7 additions & 2 deletions compiler/noirc_frontend/src/hir/def_map/module_data.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,13 @@ impl ModuleData {
self.declare(name, ItemVisibility::Public, id.into(), None)
}

pub fn declare_trait(&mut self, name: Ident, id: TraitId) -> Result<(), (Ident, Ident)> {
self.declare(name, ItemVisibility::Public, ModuleDefId::TraitId(id), None)
pub fn declare_trait(
&mut self,
name: Ident,
visibility: ItemVisibility,
id: TraitId,
) -> Result<(), (Ident, Ident)> {
self.declare(name, visibility, ModuleDefId::TraitId(id), None)
}

pub fn declare_child_module(
Expand Down
27 changes: 16 additions & 11 deletions compiler/noirc_frontend/src/parser/parser/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use super::attributes::{attributes, validate_secondary_attributes};
use super::doc_comments::outer_doc_comments;
use super::function::{function_modifiers, function_return_type};
use super::path::path_no_turbofish;
use super::visibility::item_visibility;
use super::{
block, expression, fresh_statement, function, function_declaration_parameters, let_statement,
};
Expand Down Expand Up @@ -42,22 +43,26 @@ pub(super) fn trait_definition() -> impl NoirParser<TopLevelStatementKind> {
});

attributes()
.then(item_visibility())
.then_ignore(keyword(Keyword::Trait))
.then(ident())
.then(function::generics())
.then(where_clause())
.then(trait_body_or_error)
.validate(|((((attributes, name), generics), where_clause), items), span, emit| {
let attributes = validate_secondary_attributes(attributes, span, emit);
TopLevelStatementKind::Trait(NoirTrait {
name,
generics,
where_clause,
span,
items,
attributes,
})
})
.validate(
|(((((attributes, visibility), name), generics), where_clause), items), span, emit| {
let attributes = validate_secondary_attributes(attributes, span, emit);
TopLevelStatementKind::Trait(NoirTrait {
name,
generics,
where_clause,
span,
items,
attributes,
visibility,
})
},
)
}

fn trait_body() -> impl NoirParser<Vec<Documented<TraitItem>>> {
Expand Down
38 changes: 33 additions & 5 deletions compiler/noirc_frontend/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -514,7 +514,7 @@ fn check_trait_wrong_parameter2() {
#[test]
fn check_trait_wrong_parameter_type() {
let src = "
trait Default {
pub trait Default {
fn default(x: Field, y: NotAType) -> Field;
}
Expand Down Expand Up @@ -2996,11 +2996,11 @@ fn uses_self_type_inside_trait() {
#[test]
fn uses_self_type_in_trait_where_clause() {
let src = r#"
trait Trait {
pub trait Trait {
fn trait_func() -> bool;
}
trait Foo where Self: Trait {
pub trait Foo where Self: Trait {
fn foo(self) -> bool {
self.trait_func()
}
Expand Down Expand Up @@ -3222,7 +3222,7 @@ fn errors_on_unused_private_import() {
pub fn bar() {}
pub fn baz() {}
trait Foo {
pub trait Foo {
}
}
Expand Down Expand Up @@ -3258,7 +3258,7 @@ fn errors_on_unused_pub_crate_import() {
pub fn bar() {}
pub fn baz() {}
trait Foo {
pub trait Foo {
}
}
Expand Down Expand Up @@ -3447,6 +3447,34 @@ fn errors_on_unused_struct() {
assert_eq!(*item_type, "struct");
}

#[test]
fn errors_on_unused_trait() {
let src = r#"
trait Foo {}
trait Bar {}
pub struct Baz {
}
impl Bar for Baz {}
fn main() {
}
"#;

let errors = get_program_errors(src);
assert_eq!(errors.len(), 1);

let CompilationError::ResolverError(ResolverError::UnusedItem { ident, item_type }) =
&errors[0].0
else {
panic!("Expected an unused item error");
};

assert_eq!(ident.to_string(), "Foo");
assert_eq!(*item_type, "trait");
}

#[test]
fn constrained_reference_to_unconstrained() {
let src = r#"
Expand Down
4 changes: 3 additions & 1 deletion compiler/noirc_frontend/src/usage_tracker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,15 @@ use crate::{
ast::{Ident, ItemVisibility},
hir::def_map::ModuleId,
macros_api::StructId,
node_interner::FuncId,
node_interner::{FuncId, TraitId},
};

#[derive(Debug)]
pub enum UnusedItem {
Import,
Function(FuncId),
Struct(StructId),
Trait(TraitId),
}

impl UnusedItem {
Expand All @@ -20,6 +21,7 @@ impl UnusedItem {
UnusedItem::Import => "import",
UnusedItem::Function(_) => "function",
UnusedItem::Struct(_) => "struct",
UnusedItem::Trait(_) => "trait",
}
}
}
Expand Down
10 changes: 10 additions & 0 deletions docs/docs/noir/concepts/traits.md
Original file line number Diff line number Diff line change
Expand Up @@ -463,3 +463,13 @@ impl Default for Wrapper {
Since we have an impl for our own type, the behavior of this code will not change even if `some_library` is updated
to provide its own `impl Default for Foo`. The downside of this pattern is that it requires extra wrapping and
unwrapping of values when converting to and from the `Wrapper` and `Foo` types.

### Visibility

By default, like functions, traits are private to the module the exist in. You can use `pub`
to make the trait public or `pub(crate)` to make it public to just its crate:

```rust
// This trait is now public
pub trait Trait {}
```
2 changes: 1 addition & 1 deletion noir_stdlib/src/append.nr
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
// - `T::empty().append(x) == x`
// - `x.append(T::empty()) == x`
// docs:start:append-trait
trait Append {
pub trait Append {
fn empty() -> Self;
fn append(self, other: Self) -> Self;
}
Expand Down
2 changes: 1 addition & 1 deletion noir_stdlib/src/bigint.nr
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ impl BigInt {
}
}

trait BigField {
pub trait BigField {
fn from_le_bytes(bytes: [u8]) -> Self;
fn from_le_bytes_32(bytes: [u8; 32]) -> Self;
fn to_le_bytes(self) -> [u8];
Expand Down
4 changes: 2 additions & 2 deletions noir_stdlib/src/cmp.nr
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use crate::meta::derive_via;

#[derive_via(derive_eq)]
// docs:start:eq-trait
trait Eq {
pub trait Eq {
fn eq(self, other: Self) -> bool;
}
// docs:end:eq-trait
Expand Down Expand Up @@ -173,7 +173,7 @@ impl Ordering {

#[derive_via(derive_ord)]
// docs:start:ord-trait
trait Ord {
pub trait Ord {
fn cmp(self, other: Self) -> Ordering;
}
// docs:end:ord-trait
Expand Down
4 changes: 2 additions & 2 deletions noir_stdlib/src/convert.nr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// docs:start:from-trait
trait From<T> {
pub trait From<T> {
fn from(input: T) -> Self;
}
// docs:end:from-trait
Expand All @@ -11,7 +11,7 @@ impl<T> From<T> for T {
}

// docs:start:into-trait
trait Into<T> {
pub trait Into<T> {
fn into(self) -> T;
}

Expand Down
2 changes: 1 addition & 1 deletion noir_stdlib/src/default.nr
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use crate::meta::derive_via;

#[derive_via(derive_default)]
// docs:start:default-trait
trait Default {
pub trait Default {
fn default() -> Self;
}
// docs:end:default-trait
Expand Down
6 changes: 3 additions & 3 deletions noir_stdlib/src/hash/mod.nr
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ pub fn poseidon2_permutation<let N: u32>(_input: [Field; N], _state_length: u32)

// Hash trait shall be implemented per type.
#[derive_via(derive_hash)]
trait Hash {
pub trait Hash {
fn hash<H>(self, state: &mut H) where H: Hasher;
}

Expand All @@ -155,14 +155,14 @@ comptime fn derive_hash(s: StructDefinition) -> Quoted {

// Hasher trait shall be implemented by algorithms to provide hash-agnostic means.
// TODO: consider making the types generic here ([u8], [Field], etc.)
trait Hasher{
pub trait Hasher {
fn finish(self) -> Field;

fn write(&mut self, input: Field);
}

// BuildHasher is a factory trait, responsible for production of specific Hasher.
trait BuildHasher<H> where H: Hasher{
pub trait BuildHasher<H> where H: Hasher {
fn build_hasher(self) -> H;
}

Expand Down
2 changes: 1 addition & 1 deletion noir_stdlib/src/meta/ctstring.nr
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ impl Append for CtString {
}

// docs:start:as-ctstring
trait AsCtString {
pub trait AsCtString {
comptime fn as_ctstring(self) -> CtString;
}
// docs:end:as-ctstring
Expand Down
12 changes: 6 additions & 6 deletions noir_stdlib/src/ops/arith.nr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// docs:start:add-trait
trait Add {
pub trait Add {
fn add(self, other: Self) -> Self;
}
// docs:end:add-trait
Expand Down Expand Up @@ -58,7 +58,7 @@ impl Add for i64 {
}

// docs:start:sub-trait
trait Sub {
pub trait Sub {
fn sub(self, other: Self) -> Self;
}
// docs:end:sub-trait
Expand Down Expand Up @@ -117,7 +117,7 @@ impl Sub for i64 {
}

// docs:start:mul-trait
trait Mul {
pub trait Mul {
fn mul(self, other: Self) -> Self;
}
// docs:end:mul-trait
Expand Down Expand Up @@ -176,7 +176,7 @@ impl Mul for i64 {
}

// docs:start:div-trait
trait Div {
pub trait Div {
fn div(self, other: Self) -> Self;
}
// docs:end:div-trait
Expand Down Expand Up @@ -235,7 +235,7 @@ impl Div for i64 {
}

// docs:start:rem-trait
trait Rem{
pub trait Rem {
fn rem(self, other: Self) -> Self;
}
// docs:end:rem-trait
Expand Down Expand Up @@ -288,7 +288,7 @@ impl Rem for i64 {
}

// docs:start:neg-trait
trait Neg {
pub trait Neg {
fn neg(self) -> Self;
}
// docs:end:neg-trait
Expand Down
Loading

0 comments on commit 5bbd9ba

Please sign in to comment.